Initial port of tynt to filter_parameterization subpackage #257

pllim · 2020-04-27T22:47:49Z

Based on bmorris3/tynt#8 . This would be the foundation for #38 .

Co-authored-by: @bmorris3

cc @perrygreenfield

TODO

Move relevant tests over from tynt.
Move relevant documentation over from tynt.
Fix test failure against older Numpy.
Add tips to improve results? See Initial port of tynt to filter_parameterization subpackage #257 (comment)
Make sure code support Quantity natively.
Make sure "to FFT" and "from FFT" round-trips.
Add tests to increase coverage, if needed.
Add more user facing documentation, if applicable.
~~Include an initial FFT table as package data with a small sample of generic filters? If we do this, should regenerate the table with the code here and compare to the one in tynt, if applicable.~~ Let's not do that this round. Maybe for the future.

synphot/filter_parameterization/tests/data/fft_test_data.fits

synphot/filter_parameterization/filter_fft.py

pllim · 2020-05-05T22:25:45Z

@bmorris3 , I tried to parameterize a filter I have at hand and the result doesn't look as good as what you have over at tynt. Any tips to improve the results? Thanks!

bmorris3 · 2020-05-06T07:34:50Z

@bmorris3 , I tried to parameterize a filter I have at hand and the result doesn't look as good as what you have over at tynt. Any tips to improve the results? Thanks!

Sure happy to help/iterate. Two things are most important for getting accurate representations of the transmittance curves:

the central wavelength should be at the center of your wavelength grid
there should be as little extra wavelength coverage as possible at small transmittances on either end of the bandpass

The ringing that you see red- and blue-ward of the bandpass is an inevitable artifact of the truncated Fourier series used to represent the bandpass, and you can get better approximations off of the bandpass if you clip the wavelength grid liberally. If you're looking to do this agnostically/algorithmically, perhaps clip the wavelength grid to anywhere where the transmittance is greater than 0.1% – I bet that will help with minimal side-effects on the synthetic photometry.

pllim · 2020-05-06T23:56:57Z

@bmorris3 , thanks! It looks much better now. 😄

pllim · 2020-05-07T19:10:41Z

@bmorris3 , I think I am done porting over what I think is relevant for now. Would you mind reviewing it? Thank you!

pllim · 2021-02-03T16:41:30Z

Status update: I rebased to remove conflict. Still awaiting final review from @bmorris3 and @eteq .

codecov · 2021-02-03T16:42:15Z

Codecov Report

Merging #257 (7fc2381) into master (5662d6f) will increase coverage by 0.16%.
The diff coverage is 97.93%.

@@            Coverage Diff             @@
##           master     #257      +/-   ##
==========================================
+ Coverage   94.29%   94.46%   +0.16%     
==========================================
  Files          14       17       +3     
  Lines        1999     2096      +97     
==========================================
+ Hits         1885     1980      +95     
- Misses        114      116       +2

Impacted Files	Coverage Δ
synphot/filter_parameterization/filter_fft.py	`96.72% <96.72%> (ø)`
synphot/compat.py	`80.00% <100.00%> (+2.22%)`	⬆️
synphot/filter_parameterization/__init__.py	`100.00% <100.00%> (ø)`
...t/filter_parameterization/tests/test_filter_fft.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5662d6f...7fc2381. Read the comment docs.

bmorris3

This looks great. I just looked it over again and I don't see anything amiss. Thanks again for giving me credit here and there 😄

pllim · 2021-02-03T23:05:18Z

Thanks, @bmorris3 ! I'll hold it open a bit longer to see if @eteq has anything to say, as Project Scientist for DATB.

eteq

Overall I like this! A few typos/rewordings inline, and two suggestions that can either be done now or made into issues for follow-on PRs.

docs/synphot/filter_par.rst

eteq · 2022-05-24T16:50:19Z

docs/synphot/filter_par.rst

+it is up to you to decide whether the results are good enough for your
+use cases or not.


Maybe we want to add an issue after this is merged to update this with a bit more useful guidance on what use cases it's good for? E.g. "approximating filter X with N coefficients yields a curve that is Y% as good as the full data set." It's a minor project to actually do that for a few filters though, so I think it's fine if we have this be a follow-on issue instead of blocking this PR.

Opened issue at #332

eteq · 2022-05-24T16:52:44Z

docs/synphot/filter_par.rst

+For this particular example using HST ACS/HRC F555W filter, perhaps 10
+parameters are not quite sufficient. Therefore, caution needs to be exercised


Maybe there should be one more snippet below that shows a reconstruction with more than 10 fft coefficients? That would also demonstrate how to actually do that for the user (I know it's in the API, but since it's coming up, might as well show it here).

I'll chuck this as follow-up issue because I have no idea what you mean anymore. I haven't thought about this for 2 years. 😆 See #333

eteq · 2022-05-24T16:55:33Z

synphot/filter_parameterization/filter_fft.py

+    fft_table['lambda_0'].unit = wave_unit
+    fft_table['delta_lambda'].unit = wave_unit
+
+    return fft_table


Maybe it also makes sense to add a convenience function filter_table_to_models function that takes a table and returns a dictionary of models for all of the table rows? This could also be a follow-on PR, though.

Opened issue at #334

[ci skip] Co-authored-by: Brett Morris <[email protected]>

Co-authored-by: Brett Morris <[email protected]>

Add NUMPY_LT_1_17 compat.

[ci skip] Co-authored-by: Erik Tollerud <[email protected]>

TST: Fix flake8 warning

pllim · 2022-05-24T21:46:14Z

Better late than never. Thanks, all!

pllim added enhancement Work In Progress spectrum labels Apr 27, 2020

pllim added this to the 0.3.1 milestone Apr 27, 2020

pllim requested a review from eteq April 27, 2020 22:47

bmorris3 reviewed Apr 30, 2020

View reviewed changes

synphot/filter_parameterization/tests/data/fft_test_data.fits Show resolved Hide resolved

bmorris3 reviewed Apr 30, 2020

View reviewed changes

synphot/filter_parameterization/filter_fft.py Outdated Show resolved Hide resolved

pllim force-pushed the tynt-poc branch from 440823b to c463322 Compare May 5, 2020 22:07

pllim force-pushed the tynt-poc branch 4 times, most recently from e471801 to dfb3ca4 Compare May 6, 2020 00:54

pllim changed the title ~~[WIP] Initial port of tynt to filter_parameterization subpackage~~ Initial port of tynt to filter_parameterization subpackage May 7, 2020

pllim removed the Work In Progress label May 7, 2020

pllim removed this from the 0.3.1 milestone Jul 17, 2020

pllim added this to the 1.1.0 milestone Jul 31, 2020

pllim force-pushed the tynt-poc branch from d2e60cd to 09a3414 Compare February 3, 2021 16:38

bmorris3 approved these changes Feb 3, 2021

View reviewed changes

pllim modified the milestones: 1.1.0, 1.2.0 Jun 23, 2021

eteq approved these changes May 24, 2022

View reviewed changes

pllim and others added 2 commits May 24, 2022 17:10

WIP: Initial port of tynt code to filter_parameterization.

3257b24

[ci skip] Co-authored-by: Brett Morris <[email protected]>

Port relevant unit tests from tynt

8fdefb9

pllim and others added 4 commits May 24, 2022 17:10

Add back analytical model and test.

b4102ca

Co-authored-by: Brett Morris <[email protected]>

Add documentation. Fix bugs.

ec1fb82

Refactor filters_to_fft_table internals.

399c0b5

Add NUMPY_LT_1_17 compat.

Apply edits from eteq

98a0108

[ci skip] Co-authored-by: Erik Tollerud <[email protected]>

pllim force-pushed the tynt-poc branch 2 times, most recently from a99b253 to e54107c Compare May 24, 2022 21:21

DOC: Move change log

7fc2381

TST: Fix flake8 warning

pllim force-pushed the tynt-poc branch from e54107c to 7fc2381 Compare May 24, 2022 21:24

pllim merged commit 2a382d7 into spacetelescope:master May 24, 2022

pllim deleted the tynt-poc branch May 24, 2022 21:40

pllim mentioned this pull request May 24, 2022

Parameterize non-HST filter curves #38

Open

eteq mentioned this pull request May 27, 2022

Include either in synphot or in the docs a way to access all the tynt-generated filters #335

Open

eteq mentioned this pull request Jun 10, 2022

Bandpass models m4opt/m4opt#56

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Initial port of tynt to filter_parameterization subpackage #257

Initial port of tynt to filter_parameterization subpackage #257

pllim commented Apr 27, 2020 •

edited

Loading

pllim commented May 5, 2020

bmorris3 commented May 6, 2020 •

edited

Loading

pllim commented May 6, 2020

pllim commented May 7, 2020

pllim commented Feb 3, 2021

codecov bot commented Feb 3, 2021 •

edited

Loading

bmorris3 left a comment

pllim commented Feb 3, 2021

eteq left a comment

eteq May 24, 2022

pllim May 24, 2022

eteq May 24, 2022

pllim May 24, 2022 •

edited

Loading

eteq May 24, 2022

pllim May 24, 2022

pllim commented May 24, 2022

		it is up to you to decide whether the results are good enough for your
		use cases or not.

		For this particular example using HST ACS/HRC F555W filter, perhaps 10
		parameters are not quite sufficient. Therefore, caution needs to be exercised

Initial port of tynt to filter_parameterization subpackage #257

Initial port of tynt to filter_parameterization subpackage #257

Conversation

pllim commented Apr 27, 2020 • edited Loading

pllim commented May 5, 2020

bmorris3 commented May 6, 2020 • edited Loading

pllim commented May 6, 2020

pllim commented May 7, 2020

pllim commented Feb 3, 2021

codecov bot commented Feb 3, 2021 • edited Loading

Codecov Report

bmorris3 left a comment

Choose a reason for hiding this comment

pllim commented Feb 3, 2021

eteq left a comment

Choose a reason for hiding this comment

eteq May 24, 2022

Choose a reason for hiding this comment

pllim May 24, 2022

Choose a reason for hiding this comment

eteq May 24, 2022

Choose a reason for hiding this comment

pllim May 24, 2022 • edited Loading

Choose a reason for hiding this comment

eteq May 24, 2022

Choose a reason for hiding this comment

pllim May 24, 2022

Choose a reason for hiding this comment

pllim commented May 24, 2022

pllim commented Apr 27, 2020 •

edited

Loading

bmorris3 commented May 6, 2020 •

edited

Loading

codecov bot commented Feb 3, 2021 •

edited

Loading

pllim May 24, 2022 •

edited

Loading