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

Toast / sotodlib mapmaking #11

Open
wants to merge 18 commits into
base: main
Choose a base branch
from
Open

Toast / sotodlib mapmaking #11

wants to merge 18 commits into from

Conversation

Wuhyun
Copy link
Contributor

@Wuhyun Wuhyun commented Feb 5, 2025

Mapmaking utilities built for toast and sotodlib data. Depends on toast and sotodlib for some functionalities.

The code contains all utilities needed to load toast and sotodlib data objects and run mapmaking. However, there are currently two independent sets of codes. These codes will be refactored to use a single combined interface in the near future.

Copy link
Member

@ASKabalan ASKabalan left a comment

Choose a reason for hiding this comment

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

I added some very high level comments
I did not go into all the details but looks great to me
Thank you very much for all of this 😃

self.clear()

@function_timer
def _exec(self, data, detectors=None, **kwargs): # type: ignore[no-untyped-def]
Copy link
Member

@ASKabalan ASKabalan Feb 5, 2025

Choose a reason for hiding this comment

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

I am not a big fan of perf measure in production code
this could be in an outside script maybe?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Agreed, removed the performance measures but allowed a custom logger for timing.

Comment on lines 13 to 19

@dataclass
class ObservationData:
Copy link
Member

Choose a reason for hiding this comment

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

I am assuming this is some static data class that it used internally
however it is still very easy to make into an actual jittable pytree in case it is passed around

Suggested change
@dataclass
class ObservationData:
@jax.tree_util.register_dataclass
@dataclass
class ObservationData:

check
https://jax.readthedocs.io/en/latest/_autosummary/jax.tree_util.register_dataclass.html

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you! I simply added this line for now. In the future, I will look into specifying meta/data fields for performance.


for i, leaf in enumerate(in_leaves_ref):
# looping through the Stokes parameters
zeros = in_leaves_ref.copy()
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 that copy should not be used and XLA would automatically make a copy if it is needed

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 the copy() is justified here to create a shallow copy of the original list of pytree leaves. Maybe @pchanial has an opinion on this (this function is mostly taken from his default as_matrix() implementation for abstract linear operators).

@@ -0,0 +1,161 @@
import argparse
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is cool! I added the 'furax-mapmaking' command (for now) by editing pyproject.toml

src/furax/obs/operators/_seds.py Outdated Show resolved Hide resolved
@sbiquard
Copy link
Member

sbiquard commented Feb 5, 2025

Squashed all modifications (before @Wuhyun's latest commits to address mypy errors and review comments) into one giant commit to avoid merge commits that were messing up the "changes" compared to main branch.

@sbiquard
Copy link
Member

sbiquard commented Feb 5, 2025

Added @Wuhyun as co-author on the mega-commit :)

@sbiquard sbiquard changed the title Toast mapmaking Toast / sotodlib mapmaking Feb 7, 2025
ValidLandscapeType = Literal['WCS', 'Healpix']


@dataclass(frozen=True)
Copy link
Member

Choose a reason for hiding this comment

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

Quels sont vraiment les motivations / bénéfices de l'immutabilité ici ? On pourrait lire une config par défaut, puis la mettre à jour ensuite partiellement.

Copy link
Member

Choose a reason for hiding this comment

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

Dans l'idée, ne pas pouvoir modifier la configuration spécifiée au départ par inadvertance.
Si l'utilisateur fournit un fichier de configuration partiel, les champs restants prendront la valeur par défaut.

src/furax/mapmaking/config.py Outdated Show resolved Hide resolved
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.

4 participants