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

U/jrbogart/snana support #70

Merged
merged 25 commits into from
Oct 11, 2023
Merged

U/jrbogart/snana support #70

merged 25 commits into from
Oct 11, 2023

Conversation

JoanneBogart
Copy link
Collaborator

Provides standard skyCatalogs API for SNANA-generated catalogs.
Expect flux calculation to be improved in the future.

Include commit recently merged to main which github thought might
conflict (but doesn't really)
2. Use "sncosmo" subdirectory of skycatalog root rather than "reorg" in
   __main__ tests because "reorg" doesn't have files for all needed healpixels
skyCatalogs - pass path of snana sed file to SnanaCollection
snana_object - move caching of sed file in SnanaCollection to separate
               routine, not __init__
	       Add temporary routine SnanaObject._read_nearest_sed
	       as a stopgap until real interpolation code can be
	       written
Note also catalog_creator was modified in previous commit to use
extinction column utilities
Copy link
Collaborator

@jchiang87 jchiang87 left a comment

Choose a reason for hiding this comment

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

It would be good to add some test code for all of these changes.

skycatalogs/objects/snana_object.py Outdated Show resolved Hide resolved
skycatalogs/objects/snana_object.py Outdated Show resolved Hide resolved
skycatalogs/objects/snana_object.py Outdated Show resolved Hide resolved
skycatalogs/objects/snana_object.py Outdated Show resolved Hide resolved
skycatalogs/objects/snana_object.py Outdated Show resolved Hide resolved
skycatalogs/objects/snana_object.py Outdated Show resolved Hide resolved
skycatalogs/objects/snana_object.py Outdated Show resolved Hide resolved
skycatalogs/objects/snana_object.py Outdated Show resolved Hide resolved
skycatalogs/objects/snana_object.py Outdated Show resolved Hide resolved
skycatalogs/objects/snana_object.py Outdated Show resolved Hide resolved
Copy link
Collaborator

@jchiang87 jchiang87 left a comment

Choose a reason for hiding this comment

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

I found one potentially fatal error, i.e., the undefined last_ix variable. Once that's fixed, I think it's ok to merge. I'd also suggest running flake8 or ruff on all of the source code since there are other undefined variables and similar issues elsewhere.

if index == 0:
mjd_ix_l = mjd_ix_u = 0
elif index == len(mjds):
mjd_ix_l = mjd_ix_u = last_ix
Copy link
Collaborator

Choose a reason for hiding this comment

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

last_ix is undefined. Looks like this should be

mjd_ix_l = mjd_ix_u = index - 1

@JoanneBogart JoanneBogart merged commit 7c68b00 into main Oct 11, 2023
1 check passed
@JoanneBogart JoanneBogart deleted the u/jrbogart/snana_support branch October 11, 2023 23:15
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