Skip to content

Fix some parsing issues with complex truth catalog entries#1083

Merged
rmjarvis merged 59 commits into
masterfrom
config_truth_fix
Jun 13, 2020
Merged

Fix some parsing issues with complex truth catalog entries#1083
rmjarvis merged 59 commits into
masterfrom
config_truth_fix

Conversation

@rmjarvis
Copy link
Copy Markdown
Member

@rmjarvis rmjarvis commented May 23, 2020

@NiallMac was having some problems getting some values into the truth catalog for DES runs. We eventually found a workaround, but this PR fixes the underlying issues to make it easier to get these kinds of items parsed properly.
cf. https://github.com/des-science/y3-wl_image_sims/issues/82

Specifically, I've made the following changes:

  1. When an '@' item appears in an Eval string, and the item hasn't been evaluated yet (so the config processing, doesn't know what type it is), then if there is only one possible type, go ahead and assume that's the type to use.
  2. For types that allow multiple value types (e.g. List, Random, etc.), automatically add parallel types that look like e.g. Random_float, Random_Angle, etc. which can utilize the above solution.
  3. Don't consider np.float64 and float to be different types when building the truth catalog. All float-y types are now float, and all int-y types are int.
  4. When replacing the @ values in an Eval string, do so in reverse sorted order to make sure e.g. @gal.index is replaced before @gal.
  5. Added better error messaging about mismatched types to make it easier to know where the problem is. (It had been fairly opaque.)
  6. Added a suggestion to use e.g. Random_float when it can't figure out what type a Random should be when it evaluates in a context where it doesn't know yet.

@rmjarvis rmjarvis requested a review from NiallMac May 23, 2020 00:00
@rmjarvis rmjarvis added this to the v2.3 milestone May 26, 2020
@rmjarvis
Copy link
Copy Markdown
Member Author

rmjarvis commented Jun 9, 2020

@NiallMac Do you mind taking a look at this? I think it solves all of the problems you'd been having with the truth field.

You don't have to check the implementation, but if you can at least look at the unit tests and make sure I hit all the things we tried that we thought should have worked but didn't, that would be much appreciated.

@rmjarvis
Copy link
Copy Markdown
Member Author

Any objection to merging this? I have more config-layer work I've been working on, so I'd like these changes to get onto master before that PR. I'll merge Friday unless I hear that anyone wants more time to review this.

@NiallMac
Copy link
Copy Markdown
Contributor

Sorry for the delay on this Mike, just got to it now. The expanded unit test looks great to me.

@rmjarvis rmjarvis added bug report Bug report config Related to the config-processing functionality. des Relevant to DES specifically, especially including the galsim.des module labels Jun 12, 2020
@rmjarvis rmjarvis merged commit cc2c83c into master Jun 13, 2020
@rmjarvis rmjarvis deleted the config_truth_fix branch June 13, 2020 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug report Bug report config Related to the config-processing functionality. des Relevant to DES specifically, especially including the galsim.des module

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants