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 LinearLocator, LogLocator from Visualization #1861

Merged
merged 2 commits into from Jun 30, 2018

Conversation

git-abhishek
Copy link
Contributor

@git-abhishek git-abhishek commented Jun 27, 2018

PR Summary

Cleanup by removing LinearLocator, LogLocator and FieldTransform.

PR Checklist

  • Code passes flake8 checker

Adding Reviewers

@ngoldbaum @Xarthisius @colinmarc

@git-abhishek
Copy link
Contributor Author

git-abhishek commented Jun 27, 2018

@ngoldbaum Does the LogLocator implementation complete and correct? One thing is that it does not allow to pass in user defined numticks.

@git-abhishek git-abhishek changed the title [WIP] Test case for LinearLocator and LogLocator Remove LinearLocator, LogLocator and FieldTransform from Visualization Jun 28, 2018
@matthewturk
Copy link
Member

I think this is a good change, but I'd go even further to get rid of the FieldTransform objects altogether and just use strings.

@ngoldbaum
Copy link
Member

And in places where you need to use the funciton (where is that happening btw?) you could either use a lambda or just define a function outside of the FieldTransform class and use the functions where they're needed.

@git-abhishek
Copy link
Contributor Author

Its happening here.

self._field_transform[field] = symlog_transform
self._field_transform[field].func = linthresh

How should this be handled, if we want self._field_transfrom[field] to be just string?
My earlier commit failed due to this error:

ERROR: yt.visualization.tests.test_plotwindow.test_symlog_colorbar

Traceback (most recent call last):
File "/home/travis/build/yt-project/yt/venv/lib/python3.6/site-packages/nose/case.py", line 198, in runTest
self.test(*self.arg)
File "/home/travis/build/yt-project/yt/yt/visualization/tests/test_plotwindow.py", line 551, in test_symlog_colorbar
plot.set_log(field, True, linthresh=0.1)
File "/home/travis/build/yt-project/yt/yt/visualization/plot_container.py", line 76, in newfunc
rv = f(*args, **kwargs)
File "/home/travis/build/yt-project/yt/yt/visualization/plot_container.py", line 219, in set_log
self._field_transform[field].func = linthresh
AttributeError: 'str' object has no attribute 'func'

@ngoldbaum
Copy link
Member

Ah I see, we do actually need the function, it gets passed to matplotlib here:

https://github.com/yt-project/yt/blob/master/yt/visualization/plot_window.py#L842-L847

So I guess we really do need both a name and a function, so it makes sense to keep the FieldTransform object. I guess we could use a tuple or something like that, but I think that would end up less clear.

I think this is fine as-is.

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.

Looks good!

@git-abhishek git-abhishek changed the title Remove LinearLocator, LogLocator and FieldTransform from Visualization Remove LinearLocator, LogLocator ~and FieldTransform~ from Visualization Jun 29, 2018
@git-abhishek git-abhishek changed the title Remove LinearLocator, LogLocator ~and FieldTransform~ from Visualization Remove LinearLocator, LogLocator from Visualization Jun 29, 2018
@ngoldbaum ngoldbaum merged commit d9e7a7d into yt-project:master Jun 30, 2018
@git-abhishek git-abhishek deleted the viz-tick-locators branch July 2, 2018 18:47
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.

None yet

3 participants