Fix open issue regarding warnings for criteria without data (Issue #4) #13
Conversation
Instead of checking for volcanological criteria without available data for the particular target volcano in pyvolcans.py, this is now done inside the `calculate_weighted_analogy_matrix()` function inside pyvolcans_func.py. In this way, we will not need to return the variable `my_volcano_data_dictionary` in the aforementioned function (NB. To be done in the next commit).
…ighted_analogy_matrix() This change implied further changes on pyvolcans.py and test_pyvolcans.py. NB. The `check_for_criteria_without_data()` function does not need to be imported in pyvolcans.py anymore.
A new, very simple function (`format_warning_message()`) has been created to handle the formatting of the warning messages, instead of using `warnings.warn()`. In the documentation of the functions that check for missing-data volcanological criteria and the occurrence of 'perfect analogues', outdated references to the raise of PyvolcansError have been deleted.
There was a problem hiding this comment.
Hi @PTierz , looking at this part now, 541-545
Odd suggestion from the link you give. It could be done in a more clear way as iterating through a dictionary with .items() returns key value pairs so you could just have this:
for key, value in my_volcano_data.items():
if value == 0:
my_volcano_list.append(key)
|
Thank you for the changes! It works as intended and described. One thing I would probably add is that it would be helpful if the message referred to the criteria in a standardized way, and used the flags:
That way the user would know what criteria we refer to and could immediately input the correct weightings. Do you think that's reasonable? Moreover, I don't know if that's a consideration or something we would like to do but for example if I provide:
I still get the warning even though I've set the eruption style and size to zero.
|
With this commit, the warning message related to volcanological criteria for which there is no available data (for the specific target volcano) is only printed if that particular criterion is used in the weighting scheme selected by the user: that is, if the weight for that particular volcanological criterion is greater than zero. Otherwise, no warning message will appear because, strictly speaking, data are available for all the selected criteria (those with weight > 0).
Hi @mobiuscreek! Many thanks for pointing this out! You're right. It could easily be simplified. I did so in commit 4f992dc. Thanks again! |
Hi @mobiuscreek ! Very many thanks for this comment as well! Regarding your first point on the formatting of the Warning message, I am not 100% sure whether the effort required to implement those changes, compared with the benefits, could be a bit overkill. What do you think? Of course, I may be missing an extremely easy fix, in which case, please feel free to implement it! :) (many thanks!). Concerning your second point, this is something that I had already thought about, and I think it makes sense implementing it. I made commit 5876584 to address the issue. Now, the Warning message should only show up when the criterion without available data has a weight > 0 in the weighting scheme selected by the user. To ensure this intended behaviour, could you please check the acceptance criteria below? Very many thanks in advance! Acceptance criteria
|
|
It looks good @PTierz. Thanks for the change. If you run the examples you mentioned above one last time, just to double check that the addition of the flags (-Sz, -St etc. ) in the warning message are correct and work in the last commit we should merge it. |
|
I've just been through this and checked the results against the updated criteria. All are matching. |
Replacing the formatwarning function allows us to define the warning style while still raising genuine warnings. This is useful because users may want to filter/suppress the messages as we have done with the FuzzyWuzzy messages. Renaming the "check_for..." functions to "warn_on..." makes their purpose more explicit.
|
I tweaked the warning code so that we can capture and handle the warnings. I also added a test and made some minor Flake8 fixes. If you are happy with the changes, I am happy for this to be merged now. |
|
Thank you very much, @mobiuscreek, @volcan01010, for all the changes! |
Description
Two outstanding tasks were not completed in Issue #4. These related to: (1) avoiding that the
calculate_weighted_analogy_matrix()function had to return a dictionary specifying whether data were available or not for each of the volcanological criteria, for a given target volcano; and (2) avoiding the need to catchPyvolcansErrorexceptions when either data were missing for a given criterion, or perfect analogues arose for a given target volcano and weighting scheme used. In these situations, warning messages could just be issued using thewarningspackage.Commits in branch
fix-open-issueare targeted at solving these issues. @mobiuscreek and @volcan01010: could you please check the boxes in Issue #4 and close it once one of you has been able to go through the acceptance criteria below and ensure that they are all satisfied? I can then let @jifarquharson know that Issue #4 has been solved, OK? Very many thanks in advance!Acceptance criteria
pyvolcans Alutuprints the following warning messages:PyVOLCANS: WARNING!!! The following volcanological criteria do not have any data available for the selected target volcano (Alutu) --> eruption_size. Please consider excluding these criteria from your weighting scheme (i.e. setting their weights to zero).pyvolcans Toliman -Ts 1prints the following warning messages:PyVOLCANS: WARNING!!! The following volcanological criteria do not have any data available for the selected target volcano (Toliman) --> eruption_size, eruption_style. Please consider excluding these criteria from your weighting scheme (i.e. setting their weights to zero).andPyVOLCANS: WARNING!!! All top analogue volcanoes have the same value of total analogy. Please be aware of possible data deficiencies and/or the use of a simplified weighting scheme (see Tierz et al., 2019, for more details).pyvolcans "La Palma"does not print any warning message.