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

Decouple spline staging from class declaration #192

Merged
merged 28 commits into from
May 16, 2023
Merged

Conversation

mlincett
Copy link
Collaborator

@mlincett mlincett commented May 11, 2023

Addressing #191 as intermediate step for #189.

This PR decouples the spline staging and instantation of the photospline services from the class declaration, so that importing the class does not trigger file writes or memory allocation by I3PhotoSplineService.

I have the kept the structure of having everything implemented in static attributes / methods, so the class is still not meant to be instantiated, but I am considering taking a step further and changing this as well.

In principle, aside from readability concerns, there is nothing preventing us to define the reconstruction traysegment as a bound method (so I have been told), so instead of:

tray.add(RecoAlgo.traysegment)

one would have:

reco = RecoAlgo(...)
tray.add(reco.traysegment, ...)

I think this would provide more flexibility as well as a natural way of implementing configurability for the reconstruction algorithm (see #188).

Copy link
Contributor

@tianluyuan tianluyuan left a comment

Choose a reason for hiding this comment

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

In general I'm in favor of not loading in data files until it's needed. I think this would cause issues for parallelization as in #140, though I'm not sure if that is still being pursued.

Perhaps @kjmeagher and @briedel can comment, or at least take note of this PR.

@ric-evans
Copy link
Member

ric-evans commented May 11, 2023

In general I'm in favor of not loading in data files until it's needed. I think this would cause issues for parallelization as in #140, though I'm not sure if that is still being pursued.

Perhaps @kjmeagher and @briedel can comment, or at least take note of this PR.

Ah yes, parallelization 😄 ewms-pilot enabled on-worker parallelization (called "multitasking") in v0.9.0 (Observation-Management-Service/ewms-pilot#28). Now that #174 has been merged, multitasking can be enabled.

Using this + multitasking + #174's updates helps to avoid the memory concerns that were addressed in #140. Of course, we'll have to test this to see what else needs to be addressed, but it's generally using a different paradigm.

I'm planning to begin work on this in June (after vacation 🌴)

@ric-evans ric-evans added enhancement New feature or request prod concern a problem with running at scale labels May 11, 2023
@kjmeagher
Copy link
Member

This is fine with me. This is basically the same thing as I suggested

@ric-evans
Copy link
Member

new issue: #194

@mlincett mlincett marked this pull request as ready for review May 12, 2023 13:03
Copy link
Member

@ric-evans ric-evans left a comment

Choose a reason for hiding this comment

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

Good improvements. I have a couple requests, mainly for coding style.

skymap_scanner/recos/__init__.py Show resolved Hide resolved
skymap_scanner/recos/dummy.py Show resolved Hide resolved
skymap_scanner/recos/millipede_wilks.py Show resolved Hide resolved
@mlincett
Copy link
Collaborator Author

There are no blocking concerns and I have manually tested millipede_wilks manually. I am going to merge.

We can then move the discussion to #189 .

@mlincett mlincett merged commit 7a51277 into main May 16, 2023
28 checks passed
@mlincett mlincett deleted the spline-staging-update branch May 16, 2023 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request prod concern a problem with running at scale
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants