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 useful printouts to CLI #10

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

Add useful printouts to CLI #10

wants to merge 5 commits into from

Conversation

Splines
Copy link
Collaborator

@Splines Splines commented Sep 1, 2023

Preview

image

For reviewers

  • Please make sure that the unicode symbol 🙌 is working fine in your console.
  • Note that this is actually not good with respect to clean architecture as now the transformer is dealing with printouts to the console. However, if one wanted to separate these concerns, we would have to modularize even further, passing variables outside the transformer and making a separate CLI file managing them and printing them. I my opinion, for the current scope, it's totally fine to just add some good old printouts directly in the respective files themselves. We should just watch out that this does not clutter the files too much in the future.

@Splines Splines added the enhancement New feature or request label Sep 1, 2023
@Splines Splines self-assigned this Sep 1, 2023
@Splines Splines marked this pull request as ready for review September 1, 2023 11:23
@Splines Splines requested a review from paul019 September 1, 2023 11:23
@@ -76,6 +76,14 @@ def analyze_and_offset_measurements(self, measurements: list[Measurement]):
self.points_offset_x = points_offset_x
self.points_offset_y = points_offset_y

print(f'SCALING')
x_equivalent = self.grid_config['num_x_tiny_blocks_per_block'] / factor_x
Copy link
Owner

Choose a reason for hiding this comment

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

I don't think, this formula is correct. I think, it should be x_equivalent = 1/scale_x.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Oh good catch, that's correct, I just fixed it.

src/drawer.py Outdated
@@ -103,7 +103,7 @@ def _draw_datapoint(self, m: Measurement):

def _draw_vertical_axis_number(self, num_grid):
num_data = self.trafo.grid_coord_to_data_label(num_grid, Axis.VERTICAL)
label = f'{num_data:.2e}'
label = f'{num_data:.2f}'
Copy link
Owner

Choose a reason for hiding this comment

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

I think, this isn't necessarily a good idea as the values can get very large or very small. The user should at least have the possibility to enable scientific notation.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You're right. I reverted this change as this has nothing to do with adding CLI print output.

@Splines Splines requested a review from paul019 September 6, 2023 22:31
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