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

Explicitly permit SCND bits that can update match MWS primaries #716

Merged
merged 14 commits into from May 2, 2021

Conversation

apcooper
Copy link
Contributor

@apcooper apcooper commented Apr 28, 2021

Explicitly permit secondary bits that can change the state of matched MWS primary targets.

Previous behaviour was to allow such updates to be driven by any target with SCND_ANY. Now checks for mwsupdate=True flag in target bit definition.

@apcooper apcooper changed the title Scnd updatemws Whitelist SCND bits that can update match MWS primaries Apr 28, 2021
@apcooper apcooper added the WIP Work in Progress; don't merge yet label Apr 28, 2021
@coveralls
Copy link

coveralls commented Apr 28, 2021

Coverage Status

Coverage decreased (-0.8%) to 58.889% when pulling 0af02f5 on scnd_updatemws into 5461c52 on master.

@sbailey
Copy link
Contributor

sbailey commented Apr 28, 2021

Thanks @apcooper . Please update the variable names and comments to avoid "whitelist" (and implied "blacklist") terminology. allow / permit / include / override are possible alternatives.

@geordie666
Copy link
Contributor

@apcooper: Just so you know, I took the updates you made in the bit-mask yaml file in this branch and ported them to the Main Survey bit-mask yaml file in my working branch. So, you won't need to also do that in this PR. In other words, I have retained your mwsupdate=True/False choices for the Main Survey in my working branch.

@apcooper
Copy link
Contributor Author

@geordie666 OK thanks for doing that. You say I won't need to do that -- I've done it already, so do I need to undo anything?

@sbailey noted, will fix.

@apcooper apcooper changed the title Whitelist SCND bits that can update match MWS primaries Explicitly permit SCND bits that can update match MWS primaries Apr 29, 2021
@geordie666
Copy link
Contributor

Yeah, if you could remove any changes in py/desitarget/data/targetmask.yaml that'd be great. I already have those changes in my working branch (and I have a lot more additional changes in my branch). It might be easier for me to merge the branches if you just undo anything in py/desitarget/data/targetmask.yaml. Thanks!

@apcooper
Copy link
Contributor Author

apcooper commented May 1, 2021

@geordie666 I think this is working -- I've tested it a bit and have corresponding unit tests, but some of them assume things are set up as expected in the targetmask.yaml, which is not the case now per your request. I can add these later once everything is merged, if needed.

The result of this PR is that the previous behavior is recovered if and only if updatemws=True.

@sbailey I've changed the terminology.

@apcooper apcooper removed the WIP Work in Progress; don't merge yet label May 1, 2021
@apcooper apcooper requested a review from geordie666 May 1, 2021 16:15
@apcooper apcooper marked this pull request as ready for review May 1, 2021 16:15
@apcooper
Copy link
Contributor Author

apcooper commented May 1, 2021

The last commit seems harmless and allows the tests to pass in without corresponding changes to targetmask.yaml, but I'm not sure if it fits with the style for the other flavor-like flags.

ps. "options" in the commit message should be "optional".

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.

Thanks @apcooper. This looks like carefully written code, as always. I checked a few basic use cases and it seems to work fine.

I have a couple of comments regarding backwards compatibility and the tangled web of PRs we've been creating. But, feel free to merge once those comments are addressed.

py/desitarget/secondary.py Outdated Show resolved Hide resolved
py/desitarget/secondary.py Show resolved Hide resolved
@geordie666
Copy link
Contributor

Looks good! Feel free to merge once tests pass.

@apcooper apcooper merged commit 2adf2dd into master May 2, 2021
@geordie666 geordie666 deleted the scnd_updatemws branch October 6, 2022 21:05
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