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 horizontal error bars and linear regression #11

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

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 ;)

README.md Outdated Show resolved Hide resolved
data/data.csv Outdated Show resolved Hide resolved
src/drawer.py Outdated Show resolved Hide resolved
src/drawer.py Outdated Show resolved Hide resolved
src/drawer.py Outdated Show resolved Hide resolved
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.

src/measurement.py Outdated Show resolved Hide resolved
src/measurement.py Show resolved Hide resolved
src/measurement.py Show resolved Hide resolved
src/transformer.py Outdated Show resolved Hide resolved
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

src/measurement.py Show resolved Hide resolved
src/measurement.py Outdated Show resolved Hide resolved
@Splines Splines added the enhancement New feature or request label Sep 6, 2023
src/linear_regressor.py Outdated Show resolved Hide resolved
@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.

2 participants