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

Dev #46

Merged
merged 6 commits into from
Oct 6, 2019
Merged

Dev #46

merged 6 commits into from
Oct 6, 2019

Conversation

cverstege
Copy link
Member

@cverstege cverstege commented Aug 20, 2019

Fixes updating the data of fit objects.
This needs more work and some help would be appreciated.

Updating the data with a different shape and performing a fit works for:

  • xy (tested)
  • xy multi (needs testing)
  • indexed (needs testing)
  • unbinned (tested)
  • histogram (tested)

@JohannesGaessler
Copy link
Collaborator

In what timeframe would you need me to review this pull request?
I'm currently short on time and will probably only get to it in September.

@cverstege
Copy link
Member Author

You don't have to review it. It would be nice to get some feedback on how to handle this problem properly. But I'll ask @dsavoiu tomorrow.

@cverstege
Copy link
Member Author

cverstege commented Sep 19, 2019

This should all be working now.
Unit tests for changing the size of the new data for indexed and XYMultifit are still needed.
Please review the code @dsavoiu @JohannesGaessler , as this is a bigger change.

@cverstege cverstege requested review from dsavoiu and JohannesGaessler and removed request for dsavoiu and JohannesGaessler September 20, 2019 10:57
Copy link
Member

@dsavoiu dsavoiu left a comment

Choose a reason for hiding this comment

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

Looks good overall! Appreciate the unit tests and the moving of functionality to the base class in particular. Since the tests are all passing, I'm going ahead with the merge.

@dsavoiu dsavoiu merged commit 94bfbd5 into master Oct 6, 2019
@dsavoiu dsavoiu deleted the dev branch October 6, 2019 20:48
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