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

YTEP-0003 conventions for ARTIO #2608

Closed
2 tasks
cavestruz opened this issue May 25, 2020 · 5 comments
Closed
2 tasks

YTEP-0003 conventions for ARTIO #2608

cavestruz opened this issue May 25, 2020 · 5 comments
Assignees
Labels
enhancement Making something better

Comments

@cavestruz
Copy link
Contributor

Bug report

Bug summary

ARTIO still has variable names that need to be modified to YTEP-0003 conventions.

  • 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()

Note: Self-assigning with @Provider10

Expected outcome

To be able to access ARTIO fields with either field name.

@welcome
Copy link

welcome bot commented May 25, 2020

Hi, and welcome to yt! Thanks for opening your first issue. We have an issue template that helps us to gather relevant information to help diagnosing and fixing the issue.

@triage-new-issues triage-new-issues bot added the triage Triage needed label May 25, 2020
@neutrinoceros neutrinoceros added the enhancement Making something better label May 26, 2020
@triage-new-issues triage-new-issues bot removed the triage Triage needed label May 26, 2020
@cavestruz
Copy link
Contributor Author

Hi @neutrinoceros,

@Provider10 has almost finished all of the changes needed according to the checklist above in a branch off of our fork. But, we do want to make sure that folks using the other field names can easily use (and know to use) the back-compatibility functionality. Might there be 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?

Thank you!

  • Camille and Raziq

@neutrinoceros
Copy link
Member

Hi ! Can you open a PR from this branch ? it would make the review easier since the diff would immediately available.
In any case, I do have an idea how this could be implemented but I would like the opinion of more experienced yt devs on this. I'll add my idea on your PR as a comment for it to be discussed :)

@Provider10
Copy link
Contributor

This issue is addressed with PR 2613 and I think should be closed.

@neutrinoceros
Copy link
Member

Agreed, thanks for reminding me !

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

No branches or pull requests

3 participants