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

Krill inversion component sketch #321

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

Conversation

leewujung
Copy link
Member

Creating this so that it is easier to comment on code/pseudocode.

from typing import Any, Callable, Dict, Literal, Union
from numpy.typing import ArrayLike

def mae(
Copy link
Member Author

Choose a reason for hiding this comment

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

should this be mad? it's used below in prepare_optimization

# RETURNS: Dictionary with optimization parameters and minimizer
return {"parameters": params, "minimizer": minim}

def optimize_scattering_model(
Copy link
Member Author

Choose a reason for hiding this comment

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

This should be renamed to something like optimize_model_parameters, since it is not the model that gets optimized, but the parameters.

pass


class validate_pcdwba(BaseModel):
Copy link
Member Author

Choose a reason for hiding this comment

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

I think it'll be more efficient to have build a validator that works with a model class - let's talk more about this when we meet, because I had some thought on existing scattering model packages and had a discussion on this with Sven some time in Dec.

from numpy.typing import ArrayLike


def normalize_scattering_model_parameters(
Copy link
Member Author

Choose a reason for hiding this comment

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

I think this can go into optimize.py since the output of the optimizer is doing the "inversion" so can just be packaged there.

pass


def invert_population(
Copy link
Member Author

Choose a reason for hiding this comment

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

This invert_population and Sv_prediction_error are not strictly part of the "inversion" but are derived computations. Let me read through the rest of doc and see where these might go.

from ...survey import Survey


def inversion_pipeline(
Copy link
Member Author

@leewujung leewujung Jan 10, 2025

Choose a reason for hiding this comment

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

The way things are designed now are completely separate from the hake component, which is not what I envisioned. I think it is fine as an intermediate step, but ultimately a more modularized framework will be needed to make the package more useful as an open-source project, and also remove the need for most "patcher" type of functions. Let's talk more about this when we meet.

@leewujung leewujung changed the title Kril inversion component sketch Krill inversion component sketch Jan 11, 2025
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.

2 participants