Skip to content

Conversation

@paul019
Copy link
Owner

@paul019 paul019 commented Sep 4, 2023

  • change parser including parser logic (see README)
  • update README accordingly
  • update sample data accordingly
  • update Measurement class to hold errors for x- and y-value
  • update drawer to draw horizontal error bars

- change parser including parser logic (see README)
- update README accordingly
- update sample data accordingly
- update Measurement class to hold errors for x- and y-value
- update drawer to draw horizontal error bars
@paul019 paul019 requested a review from Splines September 4, 2023 11:07
@paul019 paul019 changed the title Add horizontal error bars Add horizontal error bars and linear regression Sep 4, 2023
Copy link
Collaborator

@Splines Splines left a comment

Choose a reason for hiding this comment

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

That's a nice new feature, thanks for implementing this @paul019. Here are my two cents ;)

Comment on lines +199 to +200
def get_linear_regression(self):
return do_linear_regression(self.measurements)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not too sure if transformer should be worried with the linear regression. Based on what scope we have right now for the transformer

Transforms measurements into PDF coordinates based on grid configuration. Note that analyze_and_offset_measurements must be called first.

I'd say that a linear regression does not fit in this scope. Maybe construct a linear regression directly in drawer.py in line 48? Not sure if this is a good idea though.

Copy link
Owner Author

Choose a reason for hiding this comment

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

I find it quite fitting for the transformer. Then the drawer is only worried with drawing and the transformer is there for any data processing.

Copy link
Collaborator

@Splines Splines left a comment

Choose a reason for hiding this comment

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

some more nitpicky stuff

@Splines Splines added the enhancement New feature or request label Sep 6, 2023
@Splines
Copy link
Collaborator

Splines commented Sep 7, 2023

One thing to mention: we should probably somehow note (e.g. in the release notes) that this is a breaking change since the CSV format changed and is not compatible with the previous format anymore. In a production-like environments, one would even consider writing a small script that helps converting old file formats to not alienate users, but that's a little overkill for this project I guess.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants