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

Replace dimensions that are equal but not identical. #158

Merged
merged 3 commits into from
Jul 10, 2020

Conversation

matthewturk
Copy link
Member

This closes #157 by examining the old-style dimensions, and if they are equal but not identical, replacing them. This allows them to match the dimensions checked in the unit registry later.

@matthewturk
Copy link
Member Author

Whoops, definitely not ready. Talk about a misread of the code!

@matthewturk
Copy link
Member Author

I think this version should work. The tests pass, and the yt tests pass as well, but alternate suggestions are appreciated.

@ngoldbaum
Copy link
Member

Sorry for taking a couple days to respond. I looked at this and spotted some issues but wanted to understand the issue and code a little more clearly myself so I decided to rewrite this a little bit to make it clearer for me and not respond until I had time to do that.

I added a test that fails on master using the existing old JSON unit registry already used by the tests so hopefully we won't regress on this.

I'd appreciate it if you or @Xarthisius or whoever else could verify that the change as I have it here does fix the yt issue in the same way as your original version of this pull request.

I didn't have time to run the tests locally so they might fail, if so I'll fix that ASAP and ping you. Unfortunately I need to accompany Shawna to the prosthetist this afternoon so I won't be able to do that for a few hours.

Hope you all are having a good week and enjoyed scipy :)

@Xarthisius
Copy link
Member

I checked-out +refs/pull/158/merge: and both yt test case and unyt #157 work for me now. Thanks @ngoldbaum !

@ngoldbaum ngoldbaum merged commit e02254e into yt-project:master Jul 10, 2020
Xarthisius added a commit to Xarthisius/yt that referenced this pull request Jul 13, 2020
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.

Crash with LaTeX representation and multiple unit registry loads
3 participants