Skip to content

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
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
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.

3 participants