DM-42576: Write task to compute predicted positions for an upcoming visit #229

Gerenjie · 2024-07-26T06:13:09Z

{Summary of changes. Prefix PR title with JIRA issue.}

[x ] Do unit tests pass (scons and/or stack-os-matrix)?
[x ] Did you run ap_verify.py on at least one of the standard datasets?
For changes to metrics, the print_metricvalues script from lsst.verify will be useful.
[ x] Is the Sphinx documentation up-to-date?

kfindeisen · 2024-07-29T20:28:28Z

It looks like something went wrong with the rebase -- AFAICT "Added logic for MPSkyEphemerisQueryTask" should be the only commit on the PR. I'll continue the review on just 9ae80ca, but you might want to re-rebase on your end in the meantime.

kfindeisen

Looks good, mostly style comments. Please remember to describe the PR in the title -- people also come across PRs from the GitHub side, and most can't remember arbitrary 5-digit codes. 🙂

kfindeisen · 2024-07-29T20:30:57Z

doc/lsst.ap.verify/request.rst

@@ -0,0 +1,16 @@
+.. py:currentmodule:: lsst.ap.verify
+
+.. program:: ap_verify.py


The program:: directive is unnecessary -- it's used for defining :option: links to command-line arguments.

kfindeisen · 2024-07-29T20:37:53Z

doc/lsst.ap.verify/request.rst

+mpSkyEphemerisQueryTask
+=======================
+``ap_verify`` now runs mpSkyEphemerisQuery, which includes a request to mpSky. 
+mpSky is not expected to host ephemerides for all test datasets, so it will 
+often (or always) fail. When requests fail, mpSkyEphemerisQuery should return 
+an empty DataFrame, and no detections will be associated.


Please follow the RST style guide, in particular having a space after the header and not hard-wrapping the text. Hard-wrapping makes it hard to tell what's been edited in later changes.

kfindeisen · 2024-07-29T20:40:10Z

doc/lsst.ap.verify/request.rst

+
+mpSkyEphemerisQueryTask
+=======================
+``ap_verify`` now runs mpSkyEphemerisQuery, which includes a request to mpSky. 


Strictly speaking, ap_verify is just a framework for running preconfigured pipelines on preconfigured data sets, and has nothing to do with the pipeline contents (except for the APDB, out of necessity). How about:

Suggested change

``ap_verify`` now runs mpSkyEphemerisQuery, which includes a request to mpSky.

The AP pipelines now run :py:class:`lsst.ap.association.MPSkyEphemerisQueryTask`, which includes a request to mpSky.

An :lsst-task: role would be more appropriate, but I'm guessing you don't want to add a task topic page to ap_association at this point.

kfindeisen · 2024-07-29T20:50:41Z

doc/lsst.ap.verify/request.rst

This file is not actually linked to the documentation in any way, and users won't be able to see it. Nor is "mpSkyEphemerisQueryTask external request" really an ap_verify topic as such...

Would you be willing to expand this page into a "Known Limitations" topic, and make the MPSkyEphemerisQueryTask text a section of it?

kfindeisen · 2024-07-29T20:53:52Z

tests/test_testPipeline.py

+        #    name=VISIT, instrument=INSTRUMENT,
+        #    class_name="lsst.obs.base.instrument_tests.DummyCam",
+        # )
+


Please do not commit unused code.

kfindeisen · 2024-07-29T20:55:44Z

tests/test_testPipeline.py

        cls.repo.registry.syncDimensionData("instrument", instrumentRecord)
        butlerTests.addDataIdValue(cls.repo, "physical_filter", PHYSICAL, band=BAND)
        butlerTests.addDataIdValue(cls.repo, "subfilter", SUB_FILTER)
        butlerTests.addDataIdValue(cls.repo, "exposure", VISIT)
        butlerTests.addDataIdValue(cls.repo, "visit", VISIT)
+        butlerTests.addDataIdValue(cls.repo, "group", VISIT)


This is not correct; group is always a string, not an integer. Please make a new value (currently group names are always timestamps, but this is not guaranteed so there's no need to enforce it here).

kfindeisen · 2024-07-29T21:02:39Z

tests/test_testPipeline.py

@@ -220,6 +232,7 @@ def testMockGetTemplateTask(self):
        pipelineTests.runTestQuantum(task, self.butler, quantum, mockRun=False)

    def testMockAlardLuptonSubtractTask(self):
+


I'm ignoring most of the gratuitous whitespace changes, but this one is actually against Python conventions. I'm surprised flake8 didn't complain.

kfindeisen · 2024-07-29T22:07:15Z

tests/test_testPipeline.py

@@ -351,14 +379,14 @@ def testMockDiaPipelineTask(self):
        pipelineTests.assertValidOutput(task, result)

        self.butler.put(pandas.DataFrame(), "deepDiff_diaSrcTable", self.visitId)
-        self.butler.put(pandas.DataFrame(), "visitSsObjects", self.visitId)
+        self.butler.put(pandas.DataFrame(), "preloaded_ssObjects", self.groupId)


It's a bit strange that you can drop visitSsObjects, because the default DiaPipelineConnections still takes them as inputs. That and the fix below means this code wasn't being exercised in the first place. 😞

I'd recommend leaving this with visitSsObjects, since what we're supposed to be testing here is whether MockDiaPipelineTask is still representative of DiaPipelineTask, which has not been modified to support the new group-based dataset.

tests/test_testPipeline.py::MockTaskTestSuite::testMockDiaPipelineTask fails when this is left with visitSsObjects but does not when this is left with preloaded_SsObjects (new capitalization).

kfindeisen · 2024-08-29T22:01:15Z

Before I try to re-review this, can you please re-rebase onto main to drop everybody else's commits? Either @isullivan or I can help you with that if you need it.

Gerenjie · 2024-08-29T22:29:12Z

Before I try to re-review this, can you please re-rebase onto main to drop everybody else's commits? Either @isullivan or I can help you with that if you need it.

Done!

kfindeisen

Looks good, just a few comments about the new docs.

kfindeisen · 2024-08-29T22:40:30Z

doc/lsst.ap.verify/limitations.rst

+
+########################################
+Known Limitations 
+########################################


Sphinx requires that the header symbol lines be exactly as long as the header text. Please do run package-docs build, it will catch such things.

kfindeisen · 2024-08-29T22:50:28Z

doc/lsst.ap.verify/limitations.rst

+mpSkyEphemerisQueryTask
+=======================
+
+The AP pipelines now run :py:class:`lsst.ap.association.MPSkyEphemerisQueryTask`, which includes a request to mpSky.  mpSky is not expected to host ephemerides for all test datasets, so it will often (or always) fail. When requests fail, mpSkyEphemerisQuery should return an empty DataFrame, and no detections will be associated.


Please write each sentence on a new line; this makes it easier to spot future changes (as opposed to, say, changing one word and git diff counting it as replacing the whole paragraph).

The last sentence is out of date now. 🙂

kfindeisen · 2024-09-05T23:46:09Z

Please squash the "Formatting" commit, as it's a classic "fixup". You'll also need to rebase again; fortunately the merge conflict is easy to resolve because testPipeline.py and test_testPipeline.py can just be deleted.

kfindeisen approved these changes Jul 29, 2024

View reviewed changes

kfindeisen changed the title ~~DM-42576~~ DM-42576: Write task to compute predicted positions for an upcoming visit Jul 29, 2024

Gerenjie force-pushed the tickets/DM-42576 branch from 9ae80ca to 9c53b31 Compare August 15, 2024 17:33

Gerenjie force-pushed the tickets/DM-42576 branch from 7967af9 to 53360ff Compare August 28, 2024 08:12

Gerenjie force-pushed the tickets/DM-42576 branch from 88ac572 to e3a5e91 Compare August 29, 2024 22:28

kfindeisen approved these changes Aug 29, 2024

View reviewed changes

Gerenjie force-pushed the tickets/DM-42576 branch 2 times, most recently from 3ae9676 to 7650473 Compare September 6, 2024 18:20

Added logic for MPSkyEphemerisQueryTask

6e86cd2

Gerenjie force-pushed the tickets/DM-42576 branch from 7650473 to 6e86cd2 Compare September 6, 2024 18:21

Gerenjie merged commit 2672fb0 into main Sep 6, 2024
2 checks passed

Gerenjie deleted the tickets/DM-42576 branch September 6, 2024 18:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

DM-42576: Write task to compute predicted positions for an upcoming visit #229

DM-42576: Write task to compute predicted positions for an upcoming visit #229

Gerenjie commented Jul 26, 2024

kfindeisen commented Jul 29, 2024

kfindeisen left a comment

kfindeisen Jul 29, 2024

kfindeisen Jul 29, 2024

kfindeisen Jul 29, 2024

kfindeisen Jul 29, 2024

kfindeisen Jul 29, 2024

kfindeisen Jul 29, 2024

kfindeisen Jul 29, 2024

kfindeisen Jul 29, 2024

Gerenjie Aug 26, 2024

kfindeisen commented Aug 29, 2024

Gerenjie commented Aug 29, 2024

kfindeisen left a comment

kfindeisen Aug 29, 2024

kfindeisen Aug 29, 2024

kfindeisen Aug 29, 2024

kfindeisen commented Sep 5, 2024 •

edited

Loading

		@@ -0,0 +1,16 @@
		.. py:currentmodule:: lsst.ap.verify

		.. program:: ap_verify.py

	``ap_verify`` now runs mpSkyEphemerisQuery, which includes a request to mpSky.
	The AP pipelines now run :py:class:`lsst.ap.association.MPSkyEphemerisQueryTask`, which includes a request to mpSky.

		@@ -220,6 +232,7 @@ def testMockGetTemplateTask(self):
		pipelineTests.runTestQuantum(task, self.butler, quantum, mockRun=False)

		def testMockAlardLuptonSubtractTask(self):

DM-42576: Write task to compute predicted positions for an upcoming visit #229

DM-42576: Write task to compute predicted positions for an upcoming visit #229

Conversation

Gerenjie commented Jul 26, 2024

kfindeisen commented Jul 29, 2024

kfindeisen 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kfindeisen commented Aug 29, 2024

Gerenjie commented Aug 29, 2024

kfindeisen 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

kfindeisen commented Sep 5, 2024 • edited Loading

kfindeisen commented Sep 5, 2024 •

edited

Loading