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

remove alias of neutral fields to generic names #130

Merged
merged 16 commits into from
Jun 26, 2020

Conversation

brittonsmith
Copy link
Collaborator

In yt-4.0, the X_number_density and related fields have been re-defined to be aliases to X_nuclei_density instead of X_p0_number_density. Hence, we need to drop that old aliasing here, too. This PR removes the generic alias names and requires using p0 fields for neutral species. I think I've gotten all the right places, but am not sure, so I'll wait for some feedback on this one.

@brittonsmith
Copy link
Collaborator Author

Most of the failing tests were looking for the field names that have now been removed. Two were enzo answer tests that were using the old field names incorrectly. I'm updating the gold standard for them now.

@coveralls
Copy link

coveralls commented Apr 30, 2020

Pull Request Test Coverage Report for Build 755

  • 0 of 0 changed or added relevant lines in 0 files are covered.
  • 43 unchanged lines in 3 files lost coverage.
  • Overall coverage decreased (-0.4%) to 74.809%

Files with Coverage Reduction New Missed Lines %
/home/circleci/trident/trident/spectrum_generator.py 6 83.12%
/home/circleci/trident/trident/line_database.py 7 93.86%
/home/circleci/trident/trident/ion_balance.py 30 83.7%
Totals Coverage Status
Change from base Build 750: -0.4%
Covered Lines: 1767
Relevant Lines: 2362

💛 - Coveralls

.circleci/config.yml Outdated Show resolved Hide resolved
@brittonsmith
Copy link
Collaborator Author

@chummels, I think it would be a good idea for you to look this over before it's merged.

@chummels
Copy link
Collaborator

chummels commented May 2, 2020

I've found a number of places that you missed. I've been committing local changes to address these, but I ran into some problems with running the tests on the current gold standard, which I addressed in Issue #133 , because I've been getting a large number of failures locally and don't want to commit garbage to that messes up this PR.

@@ -70,7 +70,7 @@ commands:
command: |
source $BASH_ENV
source $HOME/venv/bin/activate
git checkout ${<< parameters.tridenttag >>}
git checkout ${<< parameters.tridenttag >>} || git checkout $TRIDENT_HEAD
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

With this change, we won't have to first set TRIDENT_GOLD to be HEAD and then change it with a direct commit after the pull request is merged. We'll still have to remember to add the tag and push it.

@brittonsmith
Copy link
Collaborator Author

@chummels, does anything else need to be done for this?

@brittonsmith brittonsmith mentioned this pull request Jun 9, 2020
Copy link
Collaborator

@chummels chummels left a comment

Choose a reason for hiding this comment

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

I found several other areas in the code where now-unnecessary checks were being made to look for aliased fields. There were also some tests and docs that were referencing the old H_number_density field. I think this now gets everything.

Note, when I run the tests locally, I see 3 test failures. The test you mentioned: test_absorption_spectrum_non_cosmo_sph, but also two tests in the test_pipelines.py test: test_enzo_small_compound and test_enzo_small_simple. But I also see these tests failing on the dev tip, so I don't think the problem was introduced by this PR.

@chummels
Copy link
Collaborator

One thing I noticed is that yt-4.0 doesn't seem to have made H_number_density alias to H_nuclei_density, at least not for the Enzo datasets. It just seems like one gets different behavior with different frontends. Do you think it's worth explicitly mapping H_number_density to H_nuclei_density for all frontends in yt4 so there isn't any random behavior?

@brittonsmith
Copy link
Collaborator Author

Looks like tests are passing. I have to go now, but feel free to merge when ready.

Copy link
Collaborator

@chummels chummels 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!

@chummels chummels merged commit 17a6584 into trident-project:master Jun 26, 2020
@brittonsmith brittonsmith deleted the aliasfields branch July 13, 2020 09:31
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.

3 participants