Add datasets for the brighter-fatter task #102

mfisherlevine · 2018-07-17T15:11:25Z

No description provided.

r-owen

As per my comment on Jira: I suggest expanding bf -to brighterFatter in these names. It makes the names clearer. It also fixes the collision with the old kernel without the need for "New" as a temporary suffix.

r-owen

A few questions. I suspect I looked at this before you had time to updated it to match changes in cp_pipe.

r-owen · 2018-08-08T22:33:48Z

policy/datasets.yaml

+    python: lsst.daf.base.PropertySet
+    tables: raw
+    template: brighterFatter_metadata/brighterFatter_metadata.boost
+brighterFatterKernelNew:


Do you need the "New" here? I thought the old name that HSC used was "bfKernel" and that "brighterFatterKernel" had not been used before.

Ack. I agree that it has not been used, and could be used in that way in principle. However, I think that while two things coexist, it is nice to have the added disambiguation of the "new" on the end, as things are confusing enough already, plus this saves having to change it. I think that once the old one is deliberately removed that we then update the name so that the One True Kernel be {{brighterFatterKernel}} as you suggest. Does that sound OK for now?

I worry that it is disruptive to have to change the name of a dataset. However, as long as the dataset name is purely internal (does not affect the name or path of persisted data) then it's probably not too bad. Go ahead.

If the dataset name affects the path or file name of the persisted data then I would be very uncomfortable changing the name later.

Well, our OOB discussion persuaded me to just change it. Doing so now :)

r-owen · 2018-08-08T22:36:09Z

policy/datasets.yaml

+    description: "The per-amplifier gains, as calculated and used by the brighter-fatter task."
+    persistable: ignored
+    storage: PickleStorage
+    python: numpy.array


Isn't this a dict of amp-or-detector-name (as a str): gain (as a float)? Though as I mentioned in cp_pipe review, if you made it a simple struct-like class then you could store the level as an attribute. Since you are using pickle storage anyway, that does not seem to be much of a hardship.

Surely the python: entry should be changed to the type of the new custom class?

Sorry, I'm confused, but it's also all sorted. I thought that this was for the kernels, not the gains. I made a class for that, and did change the python type. This is just a dict, and I have also changed it to not be a lie here. So, unless you dislike that and think this should also be a thin class (which I don't agree with, as the gains are, by definition, at the amp level, so a dict is fine), then this is all nicely done, with the nice idea expressed here applied to the kernels, not the gains.

Does that make sense?

Also, I amended the commit to save squashing later, so if you look, look here, or beware the force-push, sorry.

The python type says "numpy.array" but apparently it's a dict, so surely it should be "builtins.dict"?

Sorry, that's outdated, it does now say dict. However, I just did a plain dict rather than buitlins.dict. Does that matter? (Obviously I don't mind changing it, this is more for my education.)

You were, of course, right, I've changed now.

I suspect "builtins" is not necessary (as you say) -- if it works, leaving out "builtins" is best. That said, if omitting the entry or setting it None works then I'd go with that.

r-owen

Two questions. Once you have updated, if necessary, feel free to merge.

r-owen · 2018-09-06T21:13:05Z

policy/datasets.yaml

+    description: "The per-amplifier gains, as calculated and used by the brighter-fatter task."
+    persistable: ignored
+    storage: PickleStorage
+    python: numpy.array


Surely the python: entry should be changed to the type of the new custom class?

r-owen · 2018-09-06T21:19:13Z

policy/datasets.yaml

+plotBrighterFatterPtc:
+    description: "Plots of the photon transfer curve, as made by the brighter-fatter task."
+    persistable: None
+    python: builtins.str


Is str the right type for MatplotlibStorage? That seems a bit strange. If you are saying "arbitrary binary data" then wouldn't builtins.bytes be better?

If you can set it to None or just leave the key out, that would be best (but I don't know if it will work).

Just confirming that None does indeed work fine (@TallJimbo) , and have changed here.

TallJimbo · 2018-09-19T16:33:54Z

Just saw this fly through my email: numpy.array is not a type, it's a factory function. numpy.ndarray is the type.

mfisherlevine · 2018-09-19T17:01:49Z

Thanks Jim! I think all instances of those are gone now anyway though :)

Rename all classes, methods, instances and datasets to use full name Update some python types in the dataset definitions

mfisherlevine force-pushed the tickets/DM-13293 branch from 8fbaafa to 25fb522 Compare July 17, 2018 15:12

r-owen approved these changes Jul 26, 2018

View reviewed changes

mfisherlevine force-pushed the tickets/DM-13293 branch from 0b74bf9 to 9453ebc Compare August 6, 2018 17:52

r-owen reviewed Aug 8, 2018

View reviewed changes

mfisherlevine force-pushed the tickets/DM-13293 branch 3 times, most recently from 76f1810 to 0bf4265 Compare September 6, 2018 15:36

r-owen reviewed Sep 6, 2018

View reviewed changes

mfisherlevine force-pushed the tickets/DM-13293 branch 2 times, most recently from 20ca5d7 to a7e5382 Compare September 13, 2018 14:02

mfisherlevine force-pushed the tickets/DM-13293 branch from a7e5382 to 0fa7f4c Compare September 18, 2018 12:00

mfisherlevine force-pushed the tickets/DM-13293 branch from 0fa7f4c to 86ec3a9 Compare September 19, 2018 16:59

mfisherlevine force-pushed the tickets/DM-13293 branch from 86ec3a9 to 3435ac2 Compare September 21, 2018 16:32

mfisherlevine force-pushed the tickets/DM-13293 branch from 3435ac2 to 06032ae Compare October 10, 2018 17:54

mfisherlevine added 2 commits October 11, 2018 10:02

Add datasets for the brighter-fatter task

1632562

Update names from bf* to brighterFatter*

4fe4452

Rename all classes, methods, instances and datasets to use full name Update some python types in the dataset definitions

mfisherlevine force-pushed the tickets/DM-13293 branch from 06032ae to 4fe4452 Compare October 11, 2018 15:03

mfisherlevine merged commit 4fe4452 into master Oct 11, 2018

timj deleted the tickets/DM-13293 branch August 17, 2020 23:32

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add datasets for the brighter-fatter task #102

Add datasets for the brighter-fatter task #102

mfisherlevine commented Jul 17, 2018

r-owen left a comment

r-owen left a comment

r-owen Aug 8, 2018

mfisherlevine Sep 5, 2018

r-owen Sep 5, 2018

mfisherlevine Sep 5, 2018

r-owen Aug 8, 2018

mfisherlevine Sep 6, 2018

r-owen Sep 6, 2018

mfisherlevine Sep 7, 2018 •

edited

mfisherlevine Sep 7, 2018

r-owen Sep 7, 2018

mfisherlevine Sep 10, 2018

mfisherlevine Sep 11, 2018

r-owen Sep 19, 2018 •

edited

r-owen left a comment •

edited

r-owen Sep 6, 2018

r-owen Sep 6, 2018

TallJimbo Sep 6, 2018

mfisherlevine Sep 19, 2018

TallJimbo commented Sep 19, 2018

mfisherlevine commented Sep 19, 2018

Add datasets for the brighter-fatter task #102

Add datasets for the brighter-fatter task #102

Conversation

mfisherlevine commented Jul 17, 2018

r-owen left a comment

Choose a reason for hiding this comment

r-owen left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mfisherlevine Sep 7, 2018 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

r-owen Sep 19, 2018 • edited

Choose a reason for hiding this comment

r-owen left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TallJimbo commented Sep 19, 2018

mfisherlevine commented Sep 19, 2018

mfisherlevine Sep 7, 2018 •

edited

r-owen Sep 19, 2018 •

edited

r-owen left a comment •

edited