updating parse age to cover more formats #633

djarecka · 2021-05-11T04:32:22Z

No description provided.

codecov · 2021-05-11T04:35:08Z

Codecov Report

Merging #633 (f1f70b1) into master (02ee314) will increase coverage by 0.28%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #633      +/-   ##
==========================================
+ Coverage   83.62%   83.90%   +0.28%     
==========================================
  Files          59       59              
  Lines        5379     5474      +95     
==========================================
+ Hits         4498     4593      +95     
  Misses        881      881

Flag	Coverage Δ
unittests	`83.90% <100.00%> (+0.28%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
dandi/metadata.py	`78.50% <100.00%> (+8.50%)`	⬆️
dandi/tests/test_metadata.py	`100.00% <100.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 02ee314...f1f70b1. Read the comment docs.

djarecka · 2021-05-11T19:05:32Z

@satra @yarikoptic - i reviewed the "age" field from raw data and tried to cover all "reasonable" examples, you can see tests I added to test_parse_age and I added examples that would fail

satra · 2021-05-12T12:56:57Z

dandi/metadata.py

@@ -95,9 +95,100 @@ def get_metadata(path):
    return meta


+def _parse_iso8601(age):


here are the regex patterns we use in reproschema. for at least iso8601, we should cover the formal patterns for duration definition.

https://github.com/ReproNim/reproschema/blob/master/validation/reproschema-shacl.ttl#L327

i'm checking the regex as it seems to fail for something specific, but in general the following age patterns should be supported:

https://en.wikipedia.org/wiki/ISO_8601#Durations

<duration> # This would be pure age <start>/<duration> # This is for encoding date of birth/gestation

sorry the error was my mistake. this should help for the duration pattern:

In [20]: pat = "^P(?!$)(\\d+(?:\\.\\d+)?Y)?(\\d+(?:\\.\\d+)?M)?(\\d+(?:\\.\\d+)?W)?(\\d+(?:\\.\\d ...: +)?D)?(T(?=\\d)(\\d+(?:\\.\\d+)?H)?(\\d+(?:\\.\\d+)?M)?(\\d+(?:\\.\\d+)?S)?)?$" In [21]: re.match(pat, "P149D") Out[21]: <re.Match object; span=(0, 5), match='P149D'> In [22]: re.findall(pat, "P149D") Out[22]: [('', '', '', '149D', '', '', '', '')] In [26]: re.findall(pat, "P0Y2M1W149DT23H3M6S") Out[26]: [('0Y', '2M', '1W', '149D', 'T23H3M6S', '23H', '3M', '6S')]

ok, can move to this, thanks!

it doesn't cover "P149.D" - is that ok?

it also doesn't change "P149.0D" to "P149D", but that's perhaps is better

it's not valid iso8601 so that should be ok. i think anything that is close but not ok, could be converted and then verified with the regex. i think my above comment was mostly for ensuring that the age format we use corresponds to one of those two iso8601 cases.

if input is valid for either case:
add to age as is

try converting and repeat step 1.

if all conversions fail, raise an error and ask user to fix.

i also suspect that we need to somehow indicate in nwb the age reference (right now we are using gestat* to determine alternate or assigning birth reference). the ideal will be when we don't need to modify at all, just validate iso8601 + reference.

I would disagree that the pattern you posted covers all valid ISO8601 duration values and only valid ones. At least not how I understand the wikipedia description, e.g. "P0,5Y" is a valid value that is not covered, and "P0.6Y2D" is not a valid value that would work with the pattern. But I understand that we try to cover as many format as possible

It's just not clear to me should I be fixing values like this "P0.6Y2D" or we just keep them

yarikoptic · 2021-06-09T17:43:54Z

what is the plan for this one @satra and @djarecka ?

satra · 2021-06-09T17:50:14Z

we should include this. @djarecka - where does this stand? at least this should merge master now.

lgtm-com · 2021-06-09T20:45:29Z

This pull request introduces 1 alert when merging 397c53a into 02ee314 - view on LGTM.com

new alerts:

1 for Module is imported more than once

djarecka · 2021-06-09T22:03:08Z

I think I was checked this on some of the dandisets, but not all of the dandiset. I could still do it, but general comments are welcome. I included tests that can explain what and how is converted

yarikoptic

a testing zealot me just added a few comments, otherwise, given already a nice tests coverage seems to be LGTM

yarikoptic · 2021-06-09T22:06:20Z

dandi/metadata.py

+    if "." in matchstr.group(1):
+        qty = float(matchstr.group(1))
+        if int(qty) == qty:
+            qty = int(qty)


this code block of if isn't tests covered. I've not read code to detail yet -- do you @djarecka see a quick test case to add to tests to catch this one as well, just to be on a safe side? (code looks logical to me)

yarikoptic · 2021-06-09T22:07:05Z

dandi/metadata.py

+            raise ValueError(
+                f"ISO 8601 expected, but {age} was received,"
+                f"no rules to parse {age_rem}"
+            )


if you could add a test to cover this exception raise so whenever we change code and rename some variables exception raising doesn't raise some formatting exception?

at the end I'm not sure what case I was trying to cover here... I guess all wrong values go to the next exception (that is tested)

yarikoptic · 2021-06-09T22:09:27Z

dandi/metadata.py

+    age_rem = age.replace(matchstr.group(0), "")
+    age_rem = age_rem.strip()
+    if age_rem and age_rem[0] in [",", ";", "."]:
+        age_rem = age_rem[1:]


is there an easy test case to add to cover this line?

i think this is already covered somewhere else, so no if needed here

djarecka · 2021-06-09T22:14:43Z

@yarikoptic - I will improve the test coverage. But you (or others) can also see what "age format" should be convertible, I tried to take my cases from the current dandisets.

satra · 2021-06-09T22:42:47Z

@yarikoptic - i think this also relates to the #101 . in the ideal world we should not have to extract metadata, simply map directly and make them fix at source. but i think this kind of validation is a bigger issue for existing datasets. so we should perhaps identify issues and work with providers to fix. also age can be computed from date of birth.

@djarecka - is there a notion of this could not convert and will raise an exception?

djarecka · 2021-06-09T23:45:43Z

@satra - yes, I believe everything what is not converted will rasie an exception, but there are multiple exceptions that I added, so it won't be always the same, see here

btw. I'm still a bit confused about the usage of floats in the standard. What was the document you were reading during one of the meeting, can't find it, and don't see an answer in the summaries

satra · 2021-06-10T16:55:59Z

here is the doc: https://www.loc.gov/standards/datetime/iso-tc154-wg5_n0038_iso_wd_8601-1_2016-02-16.pdf

relevant section is 4.4.3.2

djarecka · 2021-06-10T21:43:12Z

@satra - thanks! this document says that the decimal part can be in the lowest order component (without specifying that it has to be in the Time part), so it is consistent with wikipedia. I've added an extra check, so should be fine

satra · 2021-06-10T21:48:53Z

are you allowing the "comma is preferred" mode in addition to decimals?

djarecka · 2021-06-10T22:03:03Z

@satra - I allow for comma in the iso format (test "P2DT10,5H" added), but I do convert it to the format with dot, so would return "P2DT10.5H".
I do not allow comma in the more natural human format, e.g. "3,5 weeks" is not allowed. Let me know if you want me to change it

satra · 2021-06-10T22:05:55Z

i would say for now that is fine.

djarecka added 2 commits May 11, 2021 00:30

updating parse age to cover more formats

742fcd3

linting

5f5c245

djarecka marked this pull request as draft May 11, 2021 04:32

adding parts for string with unit word; adding more tests

943cd44

satra reviewed May 12, 2021

View reviewed changes

updates for _parse_iso8601

9aedd09

satra marked this pull request as ready for review June 9, 2021 17:50

Merge branch 'master' into enh/parse_age_update

397c53a

removing one import

e674695

yarikoptic reviewed Jun 9, 2021

View reviewed changes

some cleaning and adding tests

e556783

adding decimal part check

dc8addf

adding a test with decimal part with comma

f1f70b1

satra merged commit 121ac29 into dandi:master Jun 11, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

updating parse age to cover more formats #633

updating parse age to cover more formats #633

djarecka commented May 11, 2021

codecov bot commented May 11, 2021 •

edited

Loading

djarecka commented May 11, 2021

satra May 12, 2021

satra May 12, 2021

satra May 12, 2021 •

edited

Loading

djarecka May 12, 2021

djarecka May 12, 2021

djarecka May 12, 2021

satra May 12, 2021

djarecka May 12, 2021 •

edited

Loading

djarecka May 12, 2021

yarikoptic commented Jun 9, 2021

satra commented Jun 9, 2021

lgtm-com bot commented Jun 9, 2021

djarecka commented Jun 9, 2021

yarikoptic left a comment

yarikoptic Jun 9, 2021

djarecka Jun 9, 2021

yarikoptic Jun 9, 2021

djarecka Jun 9, 2021

yarikoptic Jun 9, 2021

djarecka Jun 9, 2021

djarecka commented Jun 9, 2021

satra commented Jun 9, 2021

djarecka commented Jun 9, 2021

satra commented Jun 10, 2021

djarecka commented Jun 10, 2021

satra commented Jun 10, 2021

djarecka commented Jun 10, 2021

satra commented Jun 10, 2021

		@@ -95,9 +95,100 @@ def get_metadata(path):
		return meta


		def _parse_iso8601(age):

updating parse age to cover more formats #633

updating parse age to cover more formats #633

Conversation

djarecka commented May 11, 2021

codecov bot commented May 11, 2021 • edited Loading

Codecov Report

djarecka commented May 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

satra May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djarecka May 12, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yarikoptic commented Jun 9, 2021

satra commented Jun 9, 2021

lgtm-com bot commented Jun 9, 2021

djarecka commented Jun 9, 2021

yarikoptic left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

djarecka commented Jun 9, 2021

satra commented Jun 9, 2021

djarecka commented Jun 9, 2021

satra commented Jun 10, 2021

djarecka commented Jun 10, 2021

satra commented Jun 10, 2021

djarecka commented Jun 10, 2021

satra commented Jun 10, 2021

codecov bot commented May 11, 2021 •

edited

Loading

satra May 12, 2021 •

edited

Loading

djarecka May 12, 2021 •

edited

Loading