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.

Sign up for GitHub

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

Jump to bottom

kbs: update report-nbumbers.kb #66

Open

fschwenn wants to merge 1 commit into inspirehep:master from fschwenn:reportnumbers

fschwenn commented May 23, 2019

Removes duplicate entries.
Removes dashes from seek-terms.
Adds new report numbers.

Signed-off-by: fschwenn [email protected]

tsgit suggested changes

View reviewed changes

Contributor

tsgit left a comment

There are multiple patterns containing hyphens on the left side. I flagged only a few. Please revise. There is a duplicate pattern with ambiguous replacement.
I think we should have test cases along with this major update of patterns.

refextract/references/kbs/report-numbers.kb Outdated

+              CDF PUB PLUG UPGR PUBLIC  ---CDF-PUB-PLUG-UPGR-PUBLIC
+              CDF PUB PUBLIC            ---CDF-PUB-PUBLIC
+              CDF PUB SEC VTX PUBLIC    ---CDF-PUB-SEC-VTX-PUBLIC
+              CDF PUB SEC VTX PUBLIC    ---CDF-PUB-SEC_VTX-PUBLIC

Contributor

tsgit May 23, 2019

so the same seek term now has 2 different replacement strings. I don't think that is well defined. probably the later one overwrites the earlier one

refextract/references/kbs/report-numbers.kb Outdated

@@ @@ -362,18 +372,63 @@ SLAC PUB ---SLAC-PUB @@
               SLAC R		---SLAC-R
               SLAC TN		---SLAC-TN
               SLAC WP		---SLAC-WP
+              SLAC-CN		---SLAC-CN

Contributor

tsgit May 23, 2019

hyphens should not be in the match pattern

refextract/references/kbs/report-numbers.kb Outdated

+              SLAC-PEP-II-EE-NOTE ---SLAC-PEP-II-EE-NOTE
+              SLAC-BABAR-THESIS   ---SLAC-BABAR-THESIS
+              SLAC-BABAR-TALK     ---SLAC-BABAR-TALK
+              SLAC-SSRL           ---SLAC-SSRL

Contributor

tsgit May 23, 2019

lots of unwanted hyphens

refextract/references/kbs/report-numbers.kb Outdated

+              BNL              ---BNL
+              BNL AADD         ---BNL-AADD
+              BNL AD-RHIC      ---BNL-AD-RHIC

Contributor

tsgit May 23, 2019

hyphen


          kbs: update report-nbumbers.kb

437dbd5

 * Removes duplicate entries.

 * Removes dashes from seek-terms.

 * Adds new report numbers.

Signed-off-by: fschwenn [email protected]

fschwenn force-pushed the reportnumbers branch from ee879d2 to 437dbd5 Compare

May 24, 2019 08:20

tsgit mentioned this pull request

docextract: update report-numbers.kb #65

Closed

tsgit reviewed

View reviewed changes

refextract/references/kbs/report-numbers.kb

+              *****Tokyo Inst. Tech.*****
+              <s999?>
+              <s999?-NP>

Contributor

tsgit May 24, 2019

not sure about the hyphen here, I don't think it'll match

Contributor

michamos May 31, 2019

I don't think either, as it should have been stripped

Contributor

tsgit commented May 24, 2019 •

edited

Loading

I think it would be good to have a sample original reference line for each pattern, or at least each pattern group as a list of test cases. This would confirm the pattern does what is intended to do and no other pattern matches. At least would show when hyphens are incorrect.
e.g. similar to
https://github.com/inspirehep/refextract/blob/master/tests/test_tag.py#L132-L135
with tag_report (which you'd have to define) instead of tag_arxiv
or patterned after
https://github.com/inspirehep/refextract/blob/master/tests/test_engine.py#L102-L118 and elsewhere in that test file.
instead of multiple report_numbers I would focus on one at a time, or one pattern type at a time

Contributor

tsgit commented Jun 11, 2019

@fschwenn take a look at https://github.com/inspirehep/refextract/pull/67/files
there you see test-cases Melissa added for the Fermilab report number parsing

also, since that PR was merged, you need to rebase your PR

Author

fschwenn commented Jun 13, 2019 via email

Dear Thorsten, I think I can add test routines following Melissas example. Thanks for pointing them out to me. But I will probably fail with the rebase stuff. Would it be ok for you to just reject my previous pull request and I implement my additions on top of the actual master (in a new branch)? I know that in principle git allows much more elegant ways to handle it. Best regards, Florian

-- Florian Schwennsen Deutsches Elektronen-Synchrotron DESY Building 01 Room O1.446 phone: +49-40-8998-6190

From: "Thorsten Schwander" ***@***.***> To: "inspirehep/refextract" ***@***.***> Cc: "Florian Schwennsen" ***@***.***>, "Mention" ***@***.***> Sent: Tuesday, 11 June, 2019 19:15:13 Subject: Re: [inspirehep/refextract] kbs: update report-nbumbers.kb (#66) [ https://github.com/fschwenn | @fschwenn ] take a look at [ https://github.com/inspirehep/refextract/pull/67/files | https://github.com/inspirehep/refextract/pull/67/files ] there you see test-cases Melissa added for the Fermilab report number parsing also, since that PR was merged, you need to rebase your PR — You are receiving this because you were mentioned. Reply to this email directly, [ #66?email_source=notifications&email_token=ABZ5NE6F6G4HACBJHRCZLKLPZ7MSDA5CNFSM4HO3KUV2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXN3TEQ#issuecomment-500939154 | view it on GitHub ] , or [ https://github.com/notifications/unsubscribe-auth/ABZ5NE4WMMWBML7RHAKJTZDPZ7MSDANCNFSM4HO3KUVQ | mute the thread ] .

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet