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.

Sign up for GitHub

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

Jump to bottom

Update BAL_META #527

Merged
merged 6 commits into from
Apr 13, 2020
Merged

Update BAL_META #527

merged 6 commits into from
Apr 13, 2020

Conversation

alxogm
Copy link
Contributor

@alxogm alxogm commented Mar 23, 2020

Addressing the issue #526. A test run was made and the resultant files can be located at:
/global/cfs/cdirs/desi/science/lya/arizona_hack_bal/arizona-hack-0.4_balmeta .

The main change is to include more columns to the TRUTH extension BAL_META. See discussion on the issue.

alxogm added 3 commits March 19, 2020 23:35
- insert_bals() returns metadata for only the resultant
e94ccc9 
		qso BAL
		- BAL metatadata has now more information copied from
		the templates files
		- Saving the BAL_META is now optional
		- BAL_META resembles a BAL catalog
		- Save BAL_TEMPLATEID in TRUTH_QSO (named QSO_META before)
- removed unused lines.
bf45c95
-deleted a few comments, and removed a change in lines that in
0c5fb70 
my viewer were too long, but I think they are actually ok.
alxogm
alxogm commented Mar 23, 2020
View reviewed changes
py/desisim/io.py Outdated
meta = Table(fitsio.read(infile, ext=1, upper=True, columns=(
'SDSS_NAME', 'RA', 'DEC', 'PLATE', 'MJD', 'FIBERID')))
'SDSS_NAME', 'RA', 'DEC', 'PLATE', 'MJD', 'FIBERID','BI_CIV',
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we are using 'SDSS_NAME', 'RA', 'DEC', 'PLATE', 'MJD' columns at all, I left them here for now just to ask if it is if I remove them from here, otherwise I remove them anyway in line L59 of bal.py. @moustakas @paulmartini ?

Copy link
Member

Choose a reason for hiding this comment

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

These columns were from the original BAL template set; please remove if they're obsolete.

@alxogm
Copy link
Contributor Author

alxogm commented Mar 25, 2020

@moustakas @andreufont and @paulmartini, any comments on this PR?

BTW Travis failures seems to be unrelated to this PR.

ERROR: desisim.test.test_io (unittest.loader._FailedTest)
2728----------------------------------------------------------------------
2729ImportError: Failed to import test module: desisim.test.test_io
2730Traceback (most recent call last):
2731  File "/home/travis/miniconda/envs/test/lib/python3.5/unittest/loader.py", line 428, in _find_test_path
2732    module = self._get_module_from_name(name)
2733  File "/home/travis/miniconda/envs/test/lib/python3.5/unittest/loader.py", line 369, in _get_module_from_name
2734    __import__(name)
2735  File "/home/travis/build/desihub/desisim/py/desisim/test/test_io.py", line 8, in <module>
2736    from desisim import io
2737  File "/home/travis/build/desihub/desisim/py/desisim/io.py", line 23, in <module>
2738    import desispec.io
2739  File "/home/travis/miniconda/envs/test/lib/python3.5/site-packages/desispec/io/__init__.py", line 18, in <module>
2740    from .fibermap import read_fibermap, write_fibermap, empty_fibermap
2741  File "/home/travis/miniconda/envs/test/lib/python3.5/site-packages/desispec/io/fibermap.py", line 324
2742    expdir = f'{nightdir}/{expid:08d}'
2743                                     ^
2744SyntaxError: invalid syntax

andreufont
andreufont reviewed Mar 25, 2020
View reviewed changes
Copy link
Contributor

@andreufont andreufont left a comment

Choose a reason for hiding this comment

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

Hi @alxogm - I think the changes look reasonable, but I don't have a clear picture of how the meta data works. It would be great to wait to hear from @paulmartini and @moustakas .

moustakas
moustakas approved these changes Apr 4, 2020
View reviewed changes
Copy link
Member

@moustakas moustakas left a comment

Choose a reason for hiding this comment

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

This looks great. My only "major" comment is about coding style, but that shouldn't hold up this PR.

py/desisim/io.py Outdated
meta = Table(fitsio.read(infile, ext=1, upper=True, columns=(
'SDSS_NAME', 'RA', 'DEC', 'PLATE', 'MJD', 'FIBERID')))
'SDSS_NAME', 'RA', 'DEC', 'PLATE', 'MJD', 'FIBERID','BI_CIV',
Copy link
Member

Choose a reason for hiding this comment

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

These columns were from the original BAL template set; please remove if they're obsolete.

py/desisim/bal.py Show resolved Hide resolved
py/desisim/scripts/quickquasars.py Show resolved Hide resolved
@alxogm
Copy link
Contributor Author

alxogm commented Apr 5, 2020

Thanks @moustakas I'll fix the style issues, before merging.

@alxogm
Copy link
Contributor Author

alxogm commented Apr 7, 2020

@andreufont @paulmartini I think this is ready to merge if you agree. Thanks!
(I'll propagate the change to select_mock_targets in a separate PR).

@paulmartini
Copy link
Contributor

Okay with me.

andreufont
andreufont approved these changes Apr 8, 2020
View reviewed changes
Copy link
Contributor

@andreufont andreufont left a comment

Choose a reason for hiding this comment

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

Great, happy for it to be merged.

@sbailey sbailey merged commit b10f651 into master Apr 13, 2020
@sbailey sbailey deleted the BAL_META branch April 13, 2020 20:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@moustakas moustakas moustakas approved these changes

@andreufont andreufont andreufont approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@alxogm @paulmartini @moustakas @andreufont @sbailey