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

restore select_mock_targets to working order #788

Merged
merged 21 commits into from Jan 18, 2022
Merged

restore select_mock_targets to working order #788

merged 21 commits into from Jan 18, 2022

Conversation

moustakas
Copy link
Member

[WIP]

Building on #786, and on the heals of the major fixes in the template-generating code in desihub/desisim#559, the purpose of this PR is to restore select_mock_targets to working order (it's been broken for many months, if not a year) and to address a number of open bugs and issues, including #735, #745, #747, and others.

@moustakas moustakas added the WIP Work in Progress; don't merge yet label Jan 13, 2022
@coveralls
Copy link

coveralls commented Jan 13, 2022

Coverage Status

Coverage remained the same at 56.466% when pulling 83c8a24 on mock-fixes into d26a114 on master.

@moustakas moustakas mentioned this pull request Jan 16, 2022
@moustakas moustakas removed the WIP Work in Progress; don't merge yet label Jan 16, 2022
@moustakas
Copy link
Member Author

@geordie666 @weaverba137 @sbailey this PR is now ready for review. However, I need help setting up the unit tests to test the mock code (specifically, test/test_mock_build), which will need to bring in desisim@master as a dependency (at least until we cut a new tag).

For the record, the mock unit tests pass with a local check-out of this branch using

source /global/cfs/cdirs/desi/software/desi_environment.sh master

A couple quick notes:

  • I was originally developing and testing this branch on my laptop but I ran into this issue using desitarget.internal.sharedmem--MapReduce no longer working on Python 3.8 rainwoodman/sharedmem#24
    However, when I switched to NERSC I didn't run into this issue. Nevertheless, it'd be good to keep an eye on this change since most of the production desitarget code uses this module.

  • This PR commits a new notebook which develops Gaussian mixture models for the various target classes using DR9 photometry, doc/nb/gmm-dr9.ipynb, which may be of interest to others and for other projects.

  • There are many(!) places where select_mock_targets could be improved based on actual DESI data and updated cosmological mocks. However, that work is outside the scope of this PR, whose goal was to just get everything back into a working state.

This was referenced Jan 16, 2022
@weaverba137
Copy link
Member

For the record: it looks like desitarget already depends on desisim, and of course, desisim already depends on desitarget. Thus we have a circular dependency. It hasn't caused problems so far, but those are always a potential source of pain.

For the sharedmem issue, it would be interesting to compare what version of Python you are using on your laptop to the version at NERSC. If you're using the DESI software stack at all, I think that is still Python 3.8, which would appear to contradict the ticket.

@moustakas
Copy link
Member Author

For the sharedmem issue, it would be interesting to compare what version of Python you are using on your laptop to the version at NERSC. If you're using the DESI software stack at all, I think that is still Python 3.8, which would appear to contradict the ticket.

On my laptop I was using Python 3.8.12 vs Python 3.8.3 for the DESI software stack. I didn't dig any deeper into the issue other than to celebrate when everything "just worked" after I switched from my laptop to NERSC.

Copy link
Contributor

@geordie666 geordie666 left a comment

Choose a reason for hiding this comment

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

@moustakas: As you mention, most of your changes are specific to the mocks, which I didn't review too carefully. For the more general parts, I've added two very minor comments for you to address. I'm happy to sign off on this, though.

doc/changes.rst Outdated Show resolved Hide resolved
py/desitarget/io.py Show resolved Hide resolved
@moustakas moustakas merged commit dace344 into master Jan 18, 2022
@moustakas moustakas deleted the mock-fixes branch January 18, 2022 20:12
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.

None yet

4 participants