Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

changed all files to import constants from only tardis #1825

Merged
merged 2 commits into from
Nov 25, 2021
Merged

changed all files to import constants from only tardis #1825

merged 2 commits into from
Nov 25, 2021

Conversation

shilpiprd
Copy link
Contributor

@shilpiprd shilpiprd commented Nov 16, 2021

Description

Fixes #1672
i used the grep command in linux to locate all files which had the word constants in it.
Then i changed all files which imported constants from anywhere other than tardis. And changed them to "from tardis import constants as const or c" whichever it was initially imported as.

Motivation and context

This change does what was expected in this pr. Makes the code more reliable.

Also there was a file in ./tardis/analysis/opacities.py . It had "import astropy.constants as csts." I wasn't sure if i should change this so I didn't.
Also there was this another file in ./docs/development/developer_faq.rst
It explained as to where the constants are exported from . Since we're changing that to tardis I think this file should be changed too. I wasn't sure so I didn't.

Type of change

  • [x ] None of the above. This is a documentation change.

Checklist

- [x ] I have updated the documentation accordingly.
- [ x] (optional) I have built the documentation on my fork following [the instructions](https://tardis-sn.github.io/tardis/development/documentation_preview.html).

@tardis-bot
Copy link
Contributor

Before a pull request is accepted, it must meet the following criteria:

  • Is the necessary information provided?
  • Is this a duplicate PR?
    • If a new PR is clearly a duplicate, ask how this PR is different from the original PR?
    • If this PR is about to be merged, close the original PR with a link to this new PR that solved the issue.
  • Does it pass existing tests and are new tests provided if required?
    • The test coverage should not decrease, and for new features should be close to 100%.
  • Is the code tidy?
    • No unnecessary print lines or code comments.

@codecov
Copy link

codecov bot commented Nov 16, 2021

Codecov Report

Merging #1825 (24d7024) into master (5a62ba7) will increase coverage by 0.00%.
The diff coverage is n/a.

❗ Current head 24d7024 differs from pull request most recent head ed5416e. Consider uploading reports for the commit ed5416e to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1825   +/-   ##
=======================================
  Coverage   58.03%   58.04%           
=======================================
  Files          66       66           
  Lines        6746     6747    +1     
=======================================
+ Hits         3915     3916    +1     
  Misses       2831     2831           
Impacted Files Coverage Δ
tardis/tardis/simulation/base.py 83.33% <0.00%> (+0.07%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3dd7294...ed5416e. Read the comment docs.

@shilpiprd
Copy link
Contributor Author

This is my first PR . Can someone please review

Copy link
Contributor

@chvogl chvogl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me. If you want, you can also update ./tardis/analysis/opacities.py to use from tardis import constants as const or from tardis import constants as csts for consistency.

@@ -13,7 +13,7 @@
import yaml
from tqdm.auto import tqdm

from tardis import constants
from tardis import constants as const
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change isn't strictly necessary to fix the underlying issue. Nevertheless, it should be fine to keep it.

@shilpiprd
Copy link
Contributor Author

So is there still something that I need to do? Or can I close this PR?

@chvogl chvogl requested a review from andrewfullard November 19, 2021 13:50
@chvogl
Copy link
Contributor

chvogl commented Nov 19, 2021

We usually want two positive reviews before we merge a PR. For now, you can wait until someone else has reviewed the PR.

@andrewfullard
Copy link
Contributor

@shilpiprd please add your information to the mailmap file, then we can merge. Thank you for your contribution!

@shilpiprd
Copy link
Contributor Author

I think I updated the .mailmap file but I think someone needs to accept the changes I proposed.

@shilpiprd
Copy link
Contributor Author

Is this good enough to be merged ? Or is there something else I need to do here? Please preview

@DhruvSondhi
Copy link
Contributor

Hello @shilpiprd, please be patient with the merging of PRs. This is due to the fact that different functionality needs to be checked before we can merge. Additionally, other PRs may have a higher priority. 2 Approved Reviews are needed for a PR to be merged. I can see that you do have 2 approved reviews but it has not been merged. This will be merged as soon as possible.

@shilpiprd
Copy link
Contributor Author

Okay Thanks!

@epassaro
Copy link
Member

I think I updated the .mailmap file but I think someone needs to accept the changes I proposed.

No, you need to update .mailmap in this PR. Never push changes directly to master.

@chvogl chvogl merged commit c34beca into tardis-sn:master Nov 25, 2021
@shilpiprd shilpiprd deleted the constants branch November 26, 2021 13:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Inconsistent use of astropy and tardis constants
6 participants