Skip to content

Conversation

@edgarWolf
Copy link
Owner

Done in this PR

  • Fixed an error, where only a subset of given values for x_i in sample_curves were considered for the evolution x range.

@edgarWolf edgarWolf linked an issue Apr 4, 2025 that may be closed by this pull request
@edgarWolf edgarWolf requested a review from windisch April 4, 2025 11:58
Copy link
Collaborator

@windisch windisch left a comment

Choose a reason for hiding this comment

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

Good catch - looks good to me. Just a bit worried about complexity due to code duplicates. Added a suggestion - freel free to explore this further!

# Apply some random noise on the base values
x0 = base_latent_information.x0 + rng.normal(size=len(base_latent_information.x0), scale=x_scale)
y0 = base_latent_information.y0 + rng.normal(size=len(base_latent_information.y0), scale=y_scale)
x0 = base_latent_information.x0 + rng.normal(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Any idea to remove the code duplicates? We already had them, but now it gets more nasty...

One idea may be to have a latent dict which keys y0, x1, like:

latent["x0"] =  getattr(base_latent_information, 'x0') + rng.normal(
             size=len(getattr(base_latent_information, 'x0')), scale=scale["x"]
         )

What do you think?

@edgarWolf edgarWolf requested a review from windisch April 25, 2025 11:43
@edgarWolf
Copy link
Owner Author

@windisch Followed your suggestion, looks cleaner now. May also think about changing LatentInformation to hold more derivates than x2 and y2, but I think that's something for a separate issue.

y2 = base_latent_information.y2 + rng.normal(size=len(base_latent_information.y2), scale=y_scale)
latent_information.append(LatentInformation(y0, x0, y1, x1, y2, x2))
latent_dict = {}
for xi in xis:
Copy link
Collaborator

Choose a reason for hiding this comment

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

this may could also be a single for loop :-)

for v in xis+yis:

with x_scale and y_scale somehow accessed via v[0] and

scale = {'x': x_scale, 'y': y_scale}

But fine for me for now!

@edgarWolf edgarWolf merged commit c62fcd7 into main Apr 25, 2025
2 checks passed
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.

Fix x range when sampling curves

3 participants