add custom bandpasses #2581

profjsb · 2022-01-23T22:42:41Z

This PR adds two custom sncosmo.Bandpass filters to SkyPortal. It adds the filters via data in the config and loads the bandpasses ENUM with the new filter names. It also adds to the sncosmo registry so that the central wavelength is available to the plot.py color picker. This requires a migration.

This PR supercedes #2549, which contemplated using API endpoints to add bandpasses to the DB. Since adding filters will not be that common, and requires upgrading the enums, adding new filters via the config seems to be the fastest path (and least risky). This PR will be needed for the ATLAS PR #2498.

Co-authored by: jadalilleboe

skyportal/enum_types.py

mcoughlin · 2022-01-24T02:59:04Z

config.yaml.defaults

@@ -428,3 +428,21 @@ spectrum_types:
   - host
   - host_center
  default: source
+


This is nice.

stefanv

Just a few minor comments, then I think we're ready.

skyportal/enum_types.py

config.yaml.defaults

skyportal/enum_types.py

stefanv · 2022-01-26T05:10:21Z

skyportal/enum_types.py

+additional_bandpasses_names = []
+for additional_bandpasses in cfg.get('additional_bandpasses', []):
+    name = additional_bandpasses.get("name")
+    if not name:


Where do you expect to encounter this? I would expect this if you used .get("name", None) above.

if it's a malformed entry (name: or missing name), I dont want to throw an uncaught error.

skyportal/enum_types.py

Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>

…ot need

stefanv · 2022-01-26T06:31:52Z

This looks good, thanks Josh. Curious about that last commit: should we not remove duplicate elements somehow? It was a bit surprising to me, but apparently people use dicts to do this, since they're ordered now:

In [2]: keywords = ("a", "b", "a", "c")

In [3]: list(dict.fromkeys(keywords))
Out[3]: ['a', 'b', 'c']

profjsb · 2022-01-27T00:45:47Z

This looks good, thanks Josh. Curious about that last commit: should we not remove duplicate elements somehow? It was a bit surprising to me, but apparently people use dicts to do this, since they're ordered now:
In [2]: keywords = ("a", "b", "a", "c")

In [3]: list(dict.fromkeys(keywords))
Out[3]: ['a', 'b', 'c']

We're not doing anything wrong with the deduplication it's just that a) I dont think we need to and b) doing so triggers a rewrite the of the enum since the ordering of the passed list changed. The migration manager (via migra) sees this as a change when it really isn't. Since sncosmo is managing the majority of the bandpasses (and names), they are keeping a unique set (I believe bandpass names are the key to their registry dict). And high up in the new code I'm checking to make sure we dont conflict with existing names.

profjsb · 2022-01-27T16:35:30Z

The 1-2 failing tests seem to be unrelated (and they pass on my machine) but they did consistently fail on the CI so I'm not sure what to do. If these have become more flaky due to other PRs then we might perhaps move them to the flaky script. Check out:

skyportal/tests/frontend/sources_and_followup_etc/test_sources.py::test_obj_page_unsaved_source

requirements.txt

mcoughlin

Looks really good, and thanks for trying to improve that test!

mcoughlin · 2022-01-27T16:41:08Z

requirements.txt

@@ -7,7 +7,7 @@ seaborn==0.11.2
 bokeh==2.4.2
 pytest-randomly==3.10.3
 factory-boy==3.2.1
-astropy==5.0
+astropy==5.0.0


Was this meant to be added?

mcoughlin · 2022-01-27T16:41:44Z

skyportal/enum_types.py

+        log(
+            f"Additional Bandpass name={name} is already in the sncosmo registry. Skipping."
+        )
+    try:


Thanks for taking care of this!

stefanv · 2022-01-27T16:58:30Z

Are the failures here related to the new bandpasses?

add custom bandpasses

4f82a78

github-actions bot added config-change Pull requests that change the config file demo-data Pull requests that update the demo data labels Jan 23, 2022

profjsb added 2 commits January 23, 2022 15:04

fir demo data

d105285

add migration for DB

06b4414

github-actions bot added the migration migrates the database label Jan 24, 2022

profjsb added 2 commits January 23, 2022 17:55

update instrument.filters in migration script

4e837d0

update instrument.filters

30e470f

profjsb requested review from jadalilleboe and mcoughlin January 24, 2022 02:31

change the way enums are done

8c41571

mcoughlin requested changes Jan 24, 2022

View reviewed changes

use Kyung's method for alterning enum without downgrade

916fe78

jadalilleboe approved these changes Jan 25, 2022

View reviewed changes

profjsb added 4 commits January 25, 2022 14:01

Merge remote-tracking branch 'upstream/master' into add_atlas_bandpasses

85bf498

avoid adding bandpass with existing name

51dba65

make instrument add page more robust; tweak starlist test

428f46e

add more tries to flaky allocations test

5f7b517

stefanv reviewed Jan 26, 2022

View reviewed changes

profjsb and others added 4 commits January 25, 2022 21:45

Update skyportal/enum_types.py

230a9e4

Co-authored-by: Stefan van der Walt <sjvdwalt@gmail.com>

use tuple(set()) to make sure we get unique bandpass names

35e3fbd

update comment on bandpasses in config

15f01ea

undo tuple(set())-as it reorders the enum causing a migration we do n…

8c31558

…ot need

fix merge conflict in test_sources

a57fc5d

try removing mui styles

52fbef9

github-actions bot added the dependencies Pull requests that update a dependency file label Jan 27, 2022

profjsb added 2 commits January 26, 2022 17:06

restore mui/styles; pin astropy=5.0.0

f2ece36

fix merge confict

622b0f2

change alembic to latest revision

0243196

profjsb requested review from stefanv, jadalilleboe and mcoughlin January 27, 2022 16:30

mcoughlin reviewed Jan 27, 2022

View reviewed changes

requirements.txt Show resolved Hide resolved

mcoughlin approved these changes Jan 27, 2022

View reviewed changes

jadalilleboe approved these changes Jan 27, 2022

View reviewed changes

stefanv merged commit 698c62f into skyportal:master Jan 27, 2022

profjsb mentioned this pull request Jan 27, 2022

ATLAS forced photometry #2498

Merged

mcoughlin mentioned this pull request Feb 6, 2022

Generic passbands with sncosmo #2526

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add custom bandpasses #2581

add custom bandpasses #2581

profjsb commented Jan 23, 2022 •

edited

mcoughlin Jan 24, 2022

stefanv left a comment

stefanv Jan 26, 2022

profjsb Jan 26, 2022

stefanv commented Jan 26, 2022

profjsb commented Jan 27, 2022

profjsb commented Jan 27, 2022

mcoughlin left a comment

mcoughlin Jan 27, 2022

mcoughlin Jan 27, 2022

stefanv commented Jan 27, 2022

add custom bandpasses #2581

add custom bandpasses #2581

Conversation

profjsb commented Jan 23, 2022 • edited

mcoughlin Jan 24, 2022

Choose a reason for hiding this comment

stefanv left a comment

Choose a reason for hiding this comment

stefanv Jan 26, 2022

Choose a reason for hiding this comment

profjsb Jan 26, 2022

Choose a reason for hiding this comment

stefanv commented Jan 26, 2022

profjsb commented Jan 27, 2022

profjsb commented Jan 27, 2022

mcoughlin left a comment

Choose a reason for hiding this comment

mcoughlin Jan 27, 2022

Choose a reason for hiding this comment

mcoughlin Jan 27, 2022

Choose a reason for hiding this comment

stefanv commented Jan 27, 2022

profjsb commented Jan 23, 2022 •

edited