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

Plotting updates #72

Merged
merged 10 commits into from
Dec 4, 2018
Merged

Conversation

shanosborne
Copy link
Collaborator

These plotting updates come from problems found while investigating issues #1 and #71.

A summary of the changes made:

  • Made NIRSpec MSA detector channel plotting possible by plotting the channels from the NRS_FULL apertures
  • Removed NotImplementedError for siaf.py plot_detector_channels()
  • Add ax argument to aperture.py and siaf.py plot_detector_channels()
  • Renamed plot_all_apertures() showchannels argument to detector_channels in order to match plot_main_apertures()
  • Updated labeling arguments to be more logical between the siaf.py and aperture.py plot() methods
  • Updated siaf.py plot() docstring to have frame examples in lowercase since that's what the argument expects.

@shanosborne
Copy link
Collaborator Author

I also found an issue with plot_all_apertures(), where it tries to apply the **kwargs from the function definition in 2 places: aps = Siaf(instr, **kwargs) and aps.plot(clear=False, subarrays=subarrays, **kwargs).

I can either change it so there’s only 1 function that gets the **kwargs, or I can sort the kwargs based on the expected arguments of the 2 functions which would look something like:

siaf_kwargs_list = inspect.signature(Siaf).parameters.keys()
siaf_kwargs = dict((key, value) for key, value in kwargs.items() if key in siaf_kwargs_list)

However, this second method does assume that there is no overlap in the argument names for the 2 functions (which appears true for now, may not be in the future). Which method would you prefer to use to fix this issue? Do you think the kwargs will more likely be used for one of the functions and thus we can just remove the kwargs call from the other one?

@Johannes-Sahlmann
Copy link
Collaborator

Thanks for spotting this. I think **kwargs should be passed to aps.plot only (definitely not to two different methods).

@Johannes-Sahlmann
Copy link
Collaborator

PR looks good to me, I just would like the tests to pass before approving.

@shanosborne
Copy link
Collaborator Author

It looks like this warning doesn't have anything to do with my changes since it's also present in my master branch. But I'll look into what's causing it.

Copy link
Collaborator

@Johannes-Sahlmann Johannes-Sahlmann left a comment

Choose a reason for hiding this comment

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

Thanks for fixing the pytest warning more elegantly than I would have done.
I am not sure I actually understand how that fixture works now, but I'll look into this at a later time.

@Johannes-Sahlmann Johannes-Sahlmann merged commit 3a4cceb into spacetelescope:master Dec 4, 2018
@shanosborne shanosborne deleted the issue1 branch September 18, 2019 13:34
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.

2 participants