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

Discussion: Moving to synphot #8

Closed
pllim opened this issue Apr 21, 2020 · 4 comments
Closed

Discussion: Moving to synphot #8

pllim opened this issue Apr 21, 2020 · 4 comments

Comments

@pllim
Copy link

pllim commented Apr 21, 2020

Hello, @bmorris3 . I just thought it would be more transparent to provide feedback here on my thoughts of how this package can be a part of synphot.

  1. data/fft.fits
    1.1 When I read it in with astropy.table.Table, the column names are not very descriptive (i.e., col0, col1, etc).
    1.2 I do not think I can accept the HST and JWST parameters until they are properly vetted by appropriate STScI instrument teams. This is because that synphot is an STScI product, so people would think the values are canonical once they go in synphot, which is not the case (yet).
  2. DownloadManager -- Similar reason to 1.2 above. STScI does not control the quality of data coming from SVO. STScI has its own data management system at https://archive.stsci.edu/hlsp/reference-atlases . Therefore, I do not think synphot can accept utils/download.py at its current state, except the actual algorithm inside fft_table().
  3. core.py -- Seems okay except the download_true_transmittance(). See 1.2 and 2 for reasoning. The accompanying tests in test_core.py also depend on what in core.py gets to move over.
  4. Documentation -- Relevant documentation can also be moved over, depending on all the points above.

Here is my proposed plan for an initial PR:

  • Move the algorithm over but without any reference to SVO downloads or the fft.fits file. Could be under synphot/<subpackage_name> directory.
  • Also move the relevant tests and documentation over.
  • I am not sure what tynt stands for. I wonder if it should be renamed in its (partial) move to synphot. If so, to what? parametrized_filters?

Follow-up work may include (either as part of the PR or separate ones):

  • Have the core code in tynt work with native synphot objects.
  • Create a new data/fft.fits using canonical input files provided by STScI. We can start small with some generic filters and then see if STScI instrument teams are interested for actual HST and JWST filters.
  • Modify download to point to STScI sites, if possible.

Open questions:

  • Is my proposal acceptable? Or is your offer an "all or nothing" agreement?
  • I am not sure what is the best way to deal with duplication of code between tynt and synphot during the transition process. Or if all the code in tynt will eventually end up on synphot.

Alternative plan:

  • If we decide that moving this to synphot is not feasible after all, I am willing to add a link to this package from the synphot documentation. Or however you wish to be cited. However, if this package goes unmaintained in the future, then the citation would be revoked (usually when it becomes unusable due to compatibility reason).

cc @eteq

Also cc @perrygreenfield in case he is still interested in parametrization of HST filter curves.

@bmorris3
Copy link
Owner

@pllim Thanks for the initial audit!

Logistical thought: given the challenges associated with making this an STScI-approved product, and given that they're largely for the benefit of STScI, do you think this might be a project for someone at STScI to work on, rather than me? I imagine close communication with the instrument teams might be necessary to get the blessings from ST that you're after. I'm happy to put together more docs on how tynt works if that might help make the project more accessible to someone other than me.

@pllim
Copy link
Author

pllim commented Apr 22, 2020

@bmorris3 , I am willing to do the partial port and give you GitHub co-author credit for the initial commit. Of course, I would also ping you for review on the synphot side. Would that be acceptable?

@bmorris3
Copy link
Owner

Of course! I don't mean to put all of the work on you, please let me know how I can be useful 😄.

@pllim
Copy link
Author

pllim commented May 24, 2022

spacetelescope/synphot_refactor#257 will go in, so I am closing this. Thank you!

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