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

Modified artio frontend fields to ytep 0003 conv: Issue/2608/artio #2613

Merged

Conversation

cavestruz
Copy link
Contributor

@cavestruz cavestruz commented May 29, 2020

PR Summary

Modified ARTIO variable names to YTEP-0003 conventions for Issue #2608

  • modify fields such as HeIII density -> He_p2_density in line 53 of yt/frontends/artio/fields.py
  • add old field names to yt/fields/field_aliases.py to allow for backwards compatibility using ds._setup_deprecated_fields()

Question: @Provider10 and I would like to make sure that folks using the other field names can easily use (and know to use) the back-compatibility functionality. We are open to any thoughts or suggestions for putting in a specific error message for ARTIO users to let them know that they are missing the if the ds._setup_deprecated_fields() if the user has not added it, which we can add before this PR is merged.

Addresses: Issue #2608

PR Checklist

  • Code passes flake8 checker
  • New features are documented, with docstrings and narrative docs
  • Adds a test for any bugs fixed. Adds tests for new features.

@welcome
Copy link

welcome bot commented May 29, 2020

Hi! Welcome, and thanks for opening this pull request. We have some guidelines for new pull requests, and soon you'll hear back about the results of our tests and continuous integration checks. Thank you for your contribution!

@neutrinoceros
Copy link
Member

Thanks you for opening this !

Question: @Provider10 and I would like to make sure that folks using the other field names can easily use (and know to use) the back-compatibility functionality. We are open to any thoughts or suggestions for putting in a specific error message for ARTIO users to let them know that they are missing the if the ds._setup_deprecated_fields() if the user has not added it, which we can add before this PR is merged.

Disclaimer: I am unfamiliar with the way we maintain support to unconventional (violating YTEP-0003) field names.
It appears that they are indeed collected in yt/fields/field_aliases.py, in which case I don't see a way to add a warning specifically for ARTIO, but I would at least suggest to add comments to the new lines to clarify they were added in support to ARTIO.

After careful examination my take on this is that the code in Dataset.setup_deprecated_fields should be revised to include the warning, but this would affect all frontends equally so I believe it goes beyond the scope of this PR.

I would like to request a second opinion from someone more experienced. @brittonsmith, since you were the one to mention the violation of YTEP-0003 originally, could you give any feedback ?

@neutrinoceros neutrinoceros added the enhancement Making something better label May 30, 2020
Copy link
Member

@matthewturk matthewturk left a comment

Choose a reason for hiding this comment

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

Thank you for doing this!

@Provider10
Copy link
Contributor

@neutrinoceros -@cavestruz and I are considering opening a new issue for the deprecated fields warning, unless someone else wanted to do that.

Also @brittonsmith and @matthewturk: can both this PR and PR #2604 be a squashed and merged to master so I can do analysis from the master branch?

@brittonsmith
Copy link
Member

Sorry for taking so long to weigh in on this. The fields in yt/fields/field_aliases.py are yt-2.x field names that we probably should have removed altogether a long time ago. That said, I think putting them there is the quickest way to getting this PR merged as soon as possible. Adding machinery to allow frontend-specific deprecated fields should be relatively straightforward, but should not hold this up.

I'm in favor of merging this and creating an issue to remind ourselves that the new fields added to _field_name_aliases should be moved to the artio frontend if and when frontend-specific machinery for deprecated fields is implemented.

@neutrinoceros, I will approve this and if you agree, you can be the third approval and merge this.

@brittonsmith
Copy link
Member

Let's also merge PR #2604 as well and maybe someone can follow up with a quick PR to add the correct field alias for H2 density. I'm pretty much out of time for today. @neutrinoceros, can you take care of this?

@neutrinoceros
Copy link
Member

neutrinoceros commented Jun 19, 2020

maybe someone can follow up with a quick PR to add the correct field alias for H2 density

not sure I understand what remains to be done actually. I'm okay to merge this however !

@neutrinoceros neutrinoceros merged commit 12189b0 into yt-project:master Jun 19, 2020
@welcome
Copy link

welcome bot commented Jun 19, 2020

Hooray! Congratulations on your first merged pull request! We hope we keep seeing you around! 🎆

@Provider10 Provider10 mentioned this pull request Jun 19, 2020
2 tasks
@Provider10 Provider10 mentioned this pull request Sep 29, 2020
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Making something better
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants