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

tickets/DM-19272 Add script and task to create skymap in gen3 #288

Merged
merged 2 commits into from May 31, 2019

Conversation

natelust
Copy link
Contributor

No description provided.

except ValueError:
self.log.info(f"Inserting SkyMap {self.config.skyMapName} with hash={skyMapHash}")
with self.butler.registry.transaction():
skyMap.register(self.config.skyMapName, self.butler.registry)
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems I'm not able to create one deepCoadd_skyMap skymap with config1 and then create another deepCoadd_skyMap skymap with config2, even with a different butler collection name. I would get lsst.daf.butler.core.registry.ConflictingDefinitionError: Existing definition for skymap entry with {skymap: deepCoadd_skyMap}.

Is this intentional? Is the idea to change the skyMapName when a different config is used?

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm... it seems changing skyMapName doesn't change the hash; the hash is determined by other config parameters.

Is it not allowed to make two skymaps to two collections using the same configs, for example,
makeGen3Skymap.py REPO collection1 and then makeGen3Skymap.py REPO collection2 ?

Copy link
Contributor

Choose a reason for hiding this comment

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

In today's standup @TallJimbo clarified that a duplicate skymap of the same set of configs (determining the hash; skyMapName not included) cannot be ingested no matter the collections. That explains the ConflictingDefinitionError I saw. It seems okay to me for now, when we each have our own sqlite3 registry. I'm not sure how it will be when we share things on one Oracle database, but probably fine for now.

@hsinfang
Copy link
Contributor

hsinfang commented May 1, 2019

Attempted to run this with oracle gave me sqlalchemy.exc.CompileError: The 'oracle' dialect with current database version settings does not support in-place multirow inserts.
It's very possible that I'm not setting up the Oracle registry correctly yet. But were you able to do this with Oracle?

@hsinfang
Copy link
Contributor

hsinfang commented May 1, 2019

The above sqlalchemy.exc.CompileError was with today's master dat_butler and skymap. If I instead used w_2019_17 + this branch, it seemed to run through. Sounds like something with DM-19271 optimization?

"but does not save it to the data repository",
dtype=bool,
default=True,
)
Copy link
Member

Choose a reason for hiding this comment

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

I don't think we need this option - if someone just wanted to create a SkyMap from a config, they wouldn't really need to use this task.

butler : `str`
Path to a gen3 butler
collection : `str`
The name of the collection which the created skymap will be inserted
Copy link
Member

Choose a reason for hiding this comment

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

I think it's probably better to accept a Butler instance in run, and actually construct it from a path and collection string in the bin script - I envision the shared logic for constructing a butler from command-line arguments to eventually move to common place, and when it does, that will reduce the need for changes to this task.

def logSkyMapInfo(self, skyMap):
"""!Log information about a sky map
@param[in] skyMap sky map (an lsst.skyMap.SkyMap)
"""
Copy link
Member

Choose a reason for hiding this comment

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

Should be a numpydoc docstring, and note that the actual class is lsst.skymap.BaseSkyMap.

Also, given the duplication with the Gen2 makeSkyMapTask, maybe we should move this code into BaseSkyMap, as a method that takes a Log instance?

@natelust natelust merged commit e2ca2c6 into master May 31, 2019
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

3 participants