Skip to content

remove hard coded prints #177

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

Closed
balerion opened this issue Oct 11, 2023 · 6 comments · Fixed by #248
Closed

remove hard coded prints #177

balerion opened this issue Oct 11, 2023 · 6 comments · Fixed by #248
Labels
enhancement New feature or request

Comments

@balerion
Copy link
Collaborator

Is your feature request related to a problem? Please describe.
Cell output sometimes gets spammed with hard coded prints, for example in the append_energy_axis function.

Describe the solution you'd like
I would like for such prints to be implemented using the logging module

Describe alternatives you've considered
I have considered the possibility of removing such prints, or suppressing prints by using

class HiddenPrints:
    def __enter__(self):
        self._original_stdout = sys.stdout
        sys.stdout = open(os.devnull, 'w')

    def __exit__(self, exc_type, exc_val, exc_tb):
        sys.stdout.close()
        sys.stdout = self._original_stdout

and then

with HiddenPrints():
    prc.append_energy_axis(calibration = calibration, binwidth=1, binning=0)

but neither of these solutions is ideal, compared to the logging module solution

Additional context

@zain-sohail
Copy link
Member

Indeed I thought about moving from prints to logging but then it has to be done all at once in entire sed. Just was never the highest priority.

@zain-sohail zain-sohail added the enhancement New feature or request label Oct 11, 2023
@rettigl
Copy link
Member

rettigl commented Oct 11, 2023

I thought it a good idea to provide feedback on what the processor functions do, in the case of the dataframe operations to give a view on the dataframe shape after their action. You can also specify preview=True to see the head of the df. What is it you don't like about it?
I would suggest we discuss this issue for the refactoring to the processor2.0 @steinnymir

@balerion
Copy link
Collaborator Author

What is it you don't like about it?
The inability to turn it off, mostly. While i agree that sometimes it is useful to get the dataframe printout, in most cases i would prefer to have the option to not have it.

It should be straightforward to go from print("") to logging.info(""), can i just go ahead and do this?

@zain-sohail
Copy link
Member

I think in general logging is the right way to go. print statements are rather rudimentary approach. With logging, we can even have logs for debugging as well. And allow users to set the verbose level as well @rettigl
@balerion you can of course make the changes and put up a PR. And we can test and discuss further there.

@steinnymir
Copy link
Member

Logging is a great idea, I am a huge fan! But its not as straight forward as changing print to logger.info("").
It needs to be correctly initialized, most likely when we generate the SedProcessor instance, and passed to all objects which want to use it. At least this is how I used it in previous projects.
It is not difficult but needs to be done correctly.

This could even add a nice feature of tracking a history of all we are doing to the metadata, additional to our current method tracking.

@steinnymir steinnymir added this to the FLASH Beamtime milestone Nov 3, 2023
This was referenced Nov 7, 2023
@steinnymir
Copy link
Member

#248 closes this, but not implementing Logger. This request is moved to #249

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 a pull request may close this issue.

4 participants