Clean up logging #45
Clean up logging #45
Conversation
This is still a bit overwhelming IMO. Could we do something like Or remove the module path since it is not very relevant to users. |
frostedoyster
requested review from
PicoCentauri
and removed request for
PicoCentauri
Yes, we can try. This something related to how hydra prints the logging. We have to overwrite their config. |
| Args: | ||
| model_capabilities: The capabilities of the model. | ||
| train_loss_0: The initial training loss. | ||
| validation_loss_0: The initial validation loss. | ||
| train_info_0: The initial training metrics. | ||
| validation_info_0: The initial validation metrics. |
There was a problem hiding this comment.
This is not how we write docstrings. Can you adjust this please?
| logger.info(logging_string) | ||
|
|
||
|
|
||
| def _get_digits(value: float) -> Tuple[int, int]: |
There was a problem hiding this comment.
Is this chatGPT code?
Looks very cumbersome.
Python has a Decimal library which does this for you. i.e
t = Decimal(1.532).normalize().as_tuple()gives:
DecimalTuple(sign=0, digits=(1, 5, 3, 2, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 0, 2, 8, 4, 2, 1, 7, 0, 9, 4, 3), exponent=-26)which I think contains all the information you need in a single line. You can event get the mantissa and the exponent if you like...
There was a problem hiding this comment.
Hmmm I don't think I can use it to do what I want... Yes, what I do is quite cumbersome but I think it's the best solution for now. The function essentially gets a number and decides what parts of the number should be printed so that there are always at least 5 significant digits
There was a problem hiding this comment.
What's wrong with %12.5e or equivalent?
There was a problem hiding this comment.
I have the strong feeling that many of the people we're dealing with won't be comfortable with exponential notation
There was a problem hiding this comment.
Yes, I would go for format specifiers: f"something {some_float:12.5e}". See documentation here: https://docs.python.org/3.11/library/string.html#formatspec
There was a problem hiding this comment.
I have the strong feeling that many of the people we're dealing with won't be comfortable with exponential notation
I'm not sure about this, but we can also use g specifiers instead.
There was a problem hiding this comment.
I agree. Maybe the g is the better option then f
| logger = logging.getLogger(__name__) | ||
|
|
||
|
|
||
| class MetricLogger: |
There was a problem hiding this comment.
You promised also testing for this in your issue. Maybe at least one test that is checking if a log is produced should be added.
There was a problem hiding this comment.
Sure, I will test after we find a way to log the way we want
| if epoch == 0: | ||
| metric_logger = MetricLogger( | ||
| model_capabilities, | ||
| train_loss, | ||
| validation_loss, | ||
| finalized_train_info, | ||
| finalized_validation_info, | ||
| ) |
There was a problem hiding this comment.
Why don't you initialize the logger before entering the training loop?
There was a problem hiding this comment.
Because it needs the values from the first epoch to format things properly
|
Personally, I would go for the following: i.e., removing both the module and the thousandth of seconds (which shouldn't be relevant during our training exercises, it clogs up the log and we can definitely do without it) |
This aims to clean up the logging, according to #39.
The resulting logger can be used by several models, so it has been extracted into
utilsBefore:
After:
📚 Documentation preview 📚: https://metatensor-models--45.org.readthedocs.build/en/45/