Skip to content

Conversation

@cpwnd
Copy link

@cpwnd cpwnd commented Sep 18, 2022

Hey I merged the feature-logging branch to the latest master changes, to be in sync with the spotpy master again. Unfortunately I forgot un-synced changes in my fork vs my local branches, therefore I pushed a new branch to my fork and created a new merge request. So this branch is now perfectly in sync with thouska/spotpy:master. This replaces #244 as merge request (I closed it already) and the preceding discussion in issue #239.

Note that writing to stdout can decrease overall performance, regardless of normal print or a logging print. The advantage of using a logging mechanism is central configuration of message pattern and log levels.

Hope this finally finds it's way into the codebase :)

@cpwnd cpwnd mentioned this pull request Sep 18, 2022
Comment on lines +212 to +215
self.logger.info("*** OPTIMIZATION SEARCH TERMINATED BECAUSE THE LIMIT")
self.logger.info("ON THE MAXIMUM NUMBER OF TRIALS ")
self.logger.info(repetitions)
self.logger.info("HAS BEEN EXCEEDED.")
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be a warning?

import tables
except ImportError:
print(
spotpylogging.get_logger("hdf5").info(
Copy link
Contributor

Choose a reason for hiding this comment

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

should be a warning IMO

print(
self.logger.debug(
"THE BEST POINT HAS IMPROVED IN LAST %d LOOPS BY LESS THAN THE USER-SPECIFIED THRESHOLD %f"
% (kstop, pcento)
Copy link
Contributor

Choose a reason for hiding this comment

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

kstop, pcento should be arguments passed to debug for deferred evaluation by logging.

):
nloop += 1
print("ComplexEvo loop #%d in progress..." % nloop)
self.logger.debug("ComplexEvo loop #%d in progress..." % nloop)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.logger.debug("ComplexEvo loop #%d in progress..." % nloop)
self.logger.debug("ComplexEvo loop #%d in progress...", nloop)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this should work this way.

icall += 1
if self.status.stop:
print(
self.logger.debug(
Copy link
Contributor

Choose a reason for hiding this comment

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

Should be warning IMO

Comment on lines +80 to +82
self.logger.info(
"Starting the SA algotrithm with " + str(repetitions) + " repetitions..."
)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
self.logger.info(
"Starting the SA algotrithm with " + str(repetitions) + " repetitions..."
)
self.logger.info("Starting the SA algotrithm with %d repetitions...", repetitions)

CL = np.vstack((CL, TL[L, :]))
IPOS = IPOS + 1
print((IPOS, ITRY))
self.logger.info((IPOS, ITRY))
Copy link
Contributor

Choose a reason for hiding this comment

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

does that work?

Copy link
Contributor

@MuellerSeb MuellerSeb left a comment

Choose a reason for hiding this comment

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

I really like this. I would suggest double checking whether to use info, warning or debug depending on the message.

Also I would double check passing variables to the logging message. It should be consistent and I would either prefer f-strings or the recommended deferred evaluation approach using the old fashioned formatting approach using %s, %d, %f and so on and passing the respective variable to the logging method.

@cpwnd
Copy link
Author

cpwnd commented Jan 23, 2023

Feel free to apply your changes. From my perspective they are reasonable, but I don't have time for this at the moment.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants