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

poppy.Instrument filter loading needs refactoring #26

Closed
josePhoenix opened this issue Dec 15, 2014 · 11 comments
Closed

poppy.Instrument filter loading needs refactoring #26

josePhoenix opened this issue Dec 15, 2014 · 11 comments
Assignees
Milestone

Comments

@josePhoenix
Copy link
Collaborator

There's some blurring of lines between WebbPSF and POPPY in the filter code in poppy.Instrument. In particular, there are references to a self._filter_files attribute (that's populated by JWInstrument) within poppy.Instrument. Also, there are special cases for Webb instruments in _getWeights (#20). Refactoring here would be a good opportunity to fix mperrin/webbpsf#28 as well.

@mperrin
Copy link
Owner

mperrin commented Feb 11, 2015

Is there still more work on this needed at this point? I know you did some refactoring of this for the WFIRST work already.

@mperrin
Copy link
Owner

mperrin commented Feb 13, 2015

Aha, yes, there is still more work needed for this. I see that the Filter named tuple is defined in webbpsf_core rather than in poppy. And the old fallback-if-no-pysynphot code path in poppy was breaking because it doesn't know about Filter named tuples.

At this point for the current release we don't have time for a whole scale refactoring of this, so I'm just going to do the minimal changes to poppy to unbreak the no-pysynphot case so it might work on Travis.

@mperrin
Copy link
Owner

mperrin commented Nov 17, 2015

is this still a live issue? I think you have addressed it in some of your work on the refactoring for the WFIRST stuff?

@josePhoenix
Copy link
Collaborator Author

Alas, I think the Filter tuple is still in the wrong project. I'll dig into this today.

@mperrin
Copy link
Owner

mperrin commented Nov 19, 2015

Let's push this to the next release - it's not critical and I don't want to add in any unnecessary changes in a rush right before this release.

@mperrin mperrin modified the milestones: 0.5, 0.4 Nov 19, 2015
@mperrin
Copy link
Owner

mperrin commented Apr 21, 2016

@josePhoenix do you think you are likely to work on this in the next month? If not let's delay it to the next milestone 0.5.1. Whichever you prefer is fine with me.

@josePhoenix
Copy link
Collaborator Author

Honestly, I have to dig back into this to see what the scope really is. I will aspire to the 0.5 release :)

@josePhoenix josePhoenix self-assigned this Apr 21, 2016
@mperrin
Copy link
Owner

mperrin commented Jun 6, 2016

Shall we punt this to a future release?

@mperrin mperrin modified the milestones: 0.5.1, 0.5 Jun 8, 2016
@mperrin
Copy link
Owner

mperrin commented Sep 6, 2016

This is an issue from almost 2 years ago, and I'm not sure it's still relevant. Is it worth keeping open or can we let it go?

@josePhoenix
Copy link
Collaborator Author

I will tell you about my idea on Thursday!

@mperrin
Copy link
Owner

mperrin commented Aug 11, 2017

Stale issue. No further action needed at this time.

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

No branches or pull requests

2 participants