-
-
Notifications
You must be signed in to change notification settings - Fork 409
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
tardis/plasma/standard_plasmas.py: old database files warning #799
tardis/plasma/standard_plasmas.py: old database files warning #799
Conversation
displays a warning when using old atomic database files
tardis/plasma/standard_plasmas.py
Outdated
try: | ||
atom_data = atomic.AtomData.from_hdf(atom_data_fname) | ||
except TypeError: | ||
logger.warn('Use new configuration for the atomic database') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@laudb you need to reraise the exception because it doesn't work with the old dataset. In this reraised exception you should suggest that the error likely occured due to the use of an old-format atomic database. Then also point to where to download a new one.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wkerzendorf okay, I think I understand what you mean, will make changes to the exception and modify the logged message
displays warning when using old version of database files - edited exception and logged message
retainer the exception and put the error message in the exception not in the logger |
Sorry reraise |
displays warning when using old version of database files - raised exception with error message
@wkerzendorf grateful for the pointers. I made the necessary change, will it suffice? |
tardis/plasma/standard_plasmas.py
Outdated
|
||
try: | ||
atom_data = atomic.AtomData.from_hdf(atom_data_fname) | ||
except: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you should never have blanket exceptions. Always catch specific exceptions - you want your code to fail if it hits an unknown problem.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@wkerzendorf okay, noted. I've made this change in the next update.
displays warning when using old version of database files - use specific exception
@wkerzendorf I've been looking at this some more, and I'm also considering this possibility:
|
@laudb you can catch the error and access the message. So catch the error - display the message and add that this error might be due to the wrong dataset. I'm being very particular there because I want you to understand the error handling better. I know it's difficult and you're doing well - push on 😉 |
displays warning when using old version of database files - print error and helpful message
tardis/plasma/standard_plasmas.py
Outdated
raise ValueError('Error might be from the use of an old-format of the atomic database,\n please see '+ \ | ||
'https://github.com/tardis-sn/tardis-refdata/tree/master/atom_data for the most recent version.') | ||
except TypeError as e: | ||
print (e, 'Error might be from the use of an old-format of the atomic database, \n' +\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you don't need the plus. every string in parenthesis is automatically concatenated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh I see, noted, thanks!
displays warning when using old version of database files - remove unneeded plus from helpful message
|
||
try: | ||
atom_data = atomic.AtomData.from_hdf(atom_data_fname) | ||
except TypeError as e: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and no \
😉
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
okay, I'll have another go.. so catch the error, access the message and display it with the helpful suggestion or possible reason
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
oh..now I see
displays warning when using old version of database files - remove unneeded \ from helpful message
@tardis-sn/tardis-core please have a look at this - I'm fine with merging |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks fine to me too!
awesome! |
displays a warning when using old atomic database files