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

add support for relative step sizes #18

Closed
wants to merge 2 commits into from
Closed

Conversation

FFroehlich
Copy link
Contributor

No description provided.

@FFroehlich FFroehlich requested review from dilpath and dweindl April 16, 2023 14:13
Copy link
Member

@dweindl dweindl left a comment

Choose a reason for hiding this comment

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

👍

Comment on lines +45 to +46
warnings.warn(f'point is zero valued in dimension {dimension},'
f'resulting in zero step.')
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
warnings.warn(f'point is zero valued in dimension {dimension},'
f'resulting in zero step.')
warnings.warn(f'point is zero valued in dimension {dimension}, '
f'resulting in zero step.')

I am wondering if this could be handled better. Provide a fall-back absolute step in case the step would be zero? Interpret the supposedly relative step as absolut? Fine as is for now.

@dweindl
Copy link
Member

dweindl commented Apr 16, 2023

@dilpath Which of the tests are expected to pass? I think the ones expected to fail should either xfail, or be moved to a separate pull request.

@dilpath
Copy link
Member

dilpath commented Apr 17, 2023

Thanks for the various PRs @FFroehlich! Unfortunately, there is a redesign of this package (the redesign branch, examples in [1]), which has various improvements, and I was waiting for feedback on it via #16 before merging. However, I didn't anticipate others submitting additional PRs in the meantime.

I intend to incorporate the work done in your PRs into redesign. Is there a time-critical element to this -- would you like to have these PRs merged into main soon? If so, I can review them now, ignoring the redesign for now.

[1] https://github.com/ICB-DCM/fiddy/blob/redesign/examples/derivative.ipynb

@FFroehlich FFroehlich changed the base branch from main to redesign April 21, 2023 09:34
@FFroehlich FFroehlich mentioned this pull request Apr 24, 2023
Base automatically changed from redesign to main May 3, 2023 15:58
@dilpath
Copy link
Member

dilpath commented May 3, 2023

done in #20

@dilpath dilpath closed this May 3, 2023
@dilpath dilpath deleted the relative_steps branch May 3, 2023 16:01
@FFroehlich FFroehlich restored the relative_steps branch May 6, 2023 10:14
@FFroehlich FFroehlich deleted the relative_steps branch May 6, 2023 10:14
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.

3 participants