Upgrade compare-metadata function #778
Conversation
# Conflicts: # extra_requirements/requirements-tests.txt # strax/context.py # strax/utils.py # tests/test_context.py
for more information, see https://pre-commit.ci
|
@WenzDaniel could you help me understand this failing test? |
|
Hej @KaraMelih thank you very much for the update I think your PR goes in a very different direction than I had in my mind. The issue we had is, that if you compare the metadata of the currently used context with some old data stored somewhere that you can only perform this comparison if the metadata of your currently used context is stored somewhere. However, I think most of the time the use case is the other way around: I have a context and I cannot load any data anymore and I would like to understand why my context is different than the one used for the stored data. So only adding a simple try/except would have been already sufficient. Sorry for all the extra work. I will look into your PR now. |
There was a problem hiding this comment.
Thanks @KaraMelih works like a charm. Maybe you can add the doc-string a little and then we can merge.
| @@ -1844,30 +1844,89 @@ def get_meta(self, run_id, target) -> dict: | |||
|
|
|||
| get_metadata = get_meta | |||
|
|
|||
| def compare_metadata(self, run_id, target, old_metadata): | |||
| def compare_metadata(self, data1, data2, return_results=False): | |||
| """Compare the metadata between two strax data. | |||
There was a problem hiding this comment.
Can you add to the doc string which explains the pintout of the comparison. I was a little confused which direction things are compared is red the first and green the second file?
| :param target: data type to get | ||
| :param old_metadata: path to metadata to compare, or a dictionary, or a tuple with another | ||
| run_id, target to compare against the metadata of the first id-target pair | ||
| :param data1, data2: either a list (tuple) of runid + target pair, or path to metadata to |
There was a problem hiding this comment.
Can you add into your doc string the examples you provided here in github. Then I think it becomes more clea, like:
"""
doc-string so far
Examples:
example code 1
example code 2
"""| metadata["lineage"] if _is_stored else self.key_for(run_id, target).lineage | ||
| ) | ||
| _lineage_hash = str(self.key_for(run_id, target)) | ||
| print(_lineage_hash) |
There was a problem hiding this comment.
Remove print statement?
| _is_stored = self.is_stored(run_id, target) | ||
| metadata = self.get_metadata(run_id, target) if _is_stored else None |
There was a problem hiding this comment.
Here you could have used a
try:
self.get_metadata(...)
except strax,DataNotStored:
....(or what ever raise we are returning in case it is not stored.) Just as information for the future
|
Btw you can ignore the failing code factor. |
for more information, see https://pre-commit.ci
What is the problem / what does the code in this PR do
The previous version of the
st.compare_metadata()was accepting one runid+target pair and one already existing metadata JSON file as was pointed here: #770 by @WenzDanielThis upgrade allows for two sets of //either// runid+target pairs or existing metadata in the form of a path to the JSON file or already loaded dictionary, to do the comparison. In case the data is not loaded anywhere (e.g. peak positions) instead of doing a full metadata comparison, it just compares the lineages.
It also allows to return of extracted metadata and lineage of both requested data for convenience, if
return_results=Trueis passed.Can you briefly describe how it works?
You get your favorite context, and call the attribute;
Can you give a minimal working example (or illustrate with a figure)?
Please include the following if applicable:
Please make sure that all automated tests have passed before asking for a review (you can save the PR as a draft otherwise).