I393 data ingestion commands: add option -F for continue mini file from #426

damhuonglan · 2020-08-07T17:28:53Z

What does this PR do?

Issue #393 states that segdtoph5 doesn't continue mini file (miniPH5_xxxxx.ph5) index from option -S first_mini. However, that option is to state the first mini file index of all. In segdtoph5 there is no option to continue mini file index from.

This PR adds option -F from_mini to continue index for mini file from, in which the mini files created will not be greater than 100GB. This option will not be associated with option -M num_mini.

The help in segdtoph5 is modified and added example as following to avoid misleading:


  -M num_mini, --num_mini=num_mini
                        Create a given number of miniPH5 files. Ex: -M 38
  -S first_mini, --first_mini=first_mini
                        The index of the first miniPH5_xxxxx.ph5 file of all.
                        Ex: -S 5
  -F from_mini, --from_mini=from_mini
                        The index to continue miniPH5_xxxxx.ph5 file from. Do
                        not associate with option -M. Ex: -F 25

This option are added to mstoph5, 130toph5, 125toph5, seg2toph5 too.

Checklist

This PR is not directly related to an existing issue (which has no PR yet).
All tests pass.
Any new or changed features have are documented.
Changes have been added to CHANGELOG.txt .
First time contributors have added your name to CONTRIBUTORS.txt .

dsentinel

This looks reasonable with few small adjustments.

dsentinel · 2020-08-18T19:20:21Z

ph5/utilities/segd2ph5.py

+                       help=("The index to continue miniPH5_xxxxx.ph5 file "
+                             "from. Do not associate with option -M. "
+                             "Ex: -F 25"),
+                       metavar="from_mini", type='int', default=None)


metavar isn't necessary for named arguments if setting to the same as the arg name

dsentinel · 2020-08-18T19:21:50Z

ph5/utilities/segd2ph5.py

+    # from_mini and num_mini must not coexist
+    if (NUM_MINI is not None) and (FROM_MINI is not None):
+        raise Exception('Option -M and option -F must not be '
+                        'used at the same time.')
+
+    if ONE_PER_MINI and (FROM_MINI is None):
+        raise Exception('Option -O must be associated with option -F.')


Mutual exclusion can be handled by argparse
https://docs.python.org/3/library/argparse.html#mutual-exclusion

dsentinel · 2020-08-18T19:23:57Z

ph5/utilities/segd2ph5.py

+                    LOGGER.error("FROM_MINI must be greater than %s, "
+                                 "the highest mini file in ph5." % highestMini)
+                    EX.ph5close()
+                    return 1


what does return 1 mean?

…to handle new errors in get_args, use sys.exit() instead of return in main()

damhuonglan · 2020-08-25T19:57:01Z

List of checks need to be done:

Argument helps and examples make sense
-M associate with -F gives error
-M still works the same as before
-F with value less than the highest mini file gives error
-F with value greater than the highest mini file will create mini file start from F

You can add any more checks that you feel need too.

hrotman-pic

It looks like the -F option permits adding same DAS to new minis, making it easier to add service runs to an existing archive without having to resubmit the entire archive. Please see Working/Holly_append_test/PR426/test1. I have not validated that test archive since that is out of scope.

Overall the help is more informative and makes more sense. One small suggestion for -M flag, I think it would be good if help said also 'Recommend using when creating a new PH5' since I think that is the intended use of -M.

…test_base.create_file_list, add help to -M

hrotman-pic · 2020-09-29T14:05:05Z

To clarify, I have only tested segdtoph5 as that was the original scope of this PR and I had a set of comparison test archives. I recommend additional reviewers for the other ingestion commands.

ascire-pic · 2020-09-29T21:45:50Z

@hrotman-pic is going to test mstoph5 and 125atoph5
@akram-pic is going to test seg2toph5 and 130toph5

damhuonglan · 2020-09-29T21:49:29Z

To clarify, I have only tested segdtoph5 as that was the original scope of this PR and I had a set of comparison test archives. I recommend additional reviewers for the other ingestion commands.

@hrotman-pic Thanks for your review. I already put your recommended help message to -M. I already added -M to other ingestion commands too.

@hrotman-pic is going to test mstoph5 and 125atoph5
@akram-pic is going to test seg2toph5 and 130toph5

All data ingestion commands can be tested in this PR (checkout branch: i393_segdtoph5_optionS) except for mstoph5 is in PR #429 (checkout branch: i393-mstoph5_optionS)

hrotman-pic

Based on the meeting, this review is requesting:
Removal of -F option for all ingestion commands changed under this PR, so that users cannot recreate the problem of data from one DAS in two or more mini files.

The changes to the help for other options, such as 'Ex: -M 38' are still useful in my view.

I am not sure about keeping 'Recommend using when creating a new PH5' for the -M option help (for all ingestion commands in this PR). If we keep it, I think it will help if the help for the -M option also says that adding new data to an existing PH5 may not create new mini files.

Going slightly out of the scope of the discussion: I think it would help standalone users to have an example of an ingestion command listed in the help, such as:
segdtoph5 -n master.ph5 -c 6 -U 18N -M 20
And to state that -S defaults to 1 if not specified in the help for -S option.

damhuonglan · 2020-10-27T21:08:19Z

replaced with PR#434

damhuonglan added 5 commits August 7, 2020 10:45

add -F FROM_MINI to create mini file with id start from FROM_MINI

8aaa847

modify help to avoid misleading

9a99e53

modify help to avoid misleading

5ecb573

add option -O, one das per mini file

4d9a265

change help -M

57430e6

dsentinel linked an issue Aug 18, 2020 that may be closed by this pull request

[BUG] data ingestion commands - flags/user control for creating new mini files (orig segdtoph5 command line not following user input) #393

Open

dsentinel requested review from ascire-pic and hrotman-pic August 18, 2020 19:11

dsentinel requested changes Aug 18, 2020

View reviewed changes

remove metavar and dest when not necessary, use OptionParser.error() …

df56446

…to handle new errors in get_args, use sys.exit() instead of return in main()

damhuonglan added 6 commits September 10, 2020 15:36

change error msg and condition for arguments M, F, O

2a791d7

remove option --onepermini because it is not necessary

192f3d3

remove option --onepermini because it is not necessary

c1c4dcb

trivial

d0b7394

move das_in_mini to test_base to be used by other testcases

9375256

change error message when from_mini >= highest_mini

5c51bfe

damhuonglan closed this Sep 16, 2020

damhuonglan reopened this Sep 16, 2020

damhuonglan closed this Sep 16, 2020

damhuonglan reopened this Sep 17, 2020

damhuonglan and others added 5 commits September 17, 2020 08:22

change error message when check from_mini in test_segd2ph5

a322b90

Merge branch 'master' into i393_segdtoph5_optionS

5ef1cc3

add option --from_mini to obspytoph5 and unittest for it

5dc854d

revert changing obspytoph5

fd8d86b

Merge branch 'master' into i393_segdtoph5_optionS

4fc30a9

hrotman-pic reviewed Sep 28, 2020

View reviewed changes

damhuonglan changed the title ~~I393 segdtoph5 add option -F for continue mini file from~~ I393 data ingestion commands: add option -F for continue mini file from Sep 28, 2020

damhuonglan added 2 commits September 28, 2020 15:48

Merge branch 'master' into i393_segdtoph5_optionS

75afb9a

change name 1302ph5 to rt1302ph5

49ad719

damhuonglan added 6 commits September 28, 2020 15:59

change getHighestMini() to get_highest_mini, create_fcntfile_list to …

fd6891a

…test_base.create_file_list, add help to -M

add function create_file_list to test_base.py

ae15033

add option --from_mini, unittest, test data for rt1302ph5

af9f7cc

add option --from_mini, unittest, test data for seg2toph5

0336c11

add option --from_mini, unittest for texan2ph5

96d0dca

clean up test_base.create_file_list

ee9e146

ascire-pic requested a review from akram-pic September 29, 2020 21:44

ascire-pic requested a review from hrotman-pic September 29, 2020 21:47

damhuonglan removed the request for review from ascire-pic October 1, 2020 20:25

hrotman-pic requested changes Oct 14, 2020

View reviewed changes

damhuonglan mentioned this pull request Oct 16, 2020

Data ingestion help #434

Merged

5 tasks

damhuonglan closed this Oct 27, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

I393 data ingestion commands: add option -F for continue mini file from #426

I393 data ingestion commands: add option -F for continue mini file from #426

damhuonglan commented Aug 7, 2020 •

edited

Loading

dsentinel left a comment

dsentinel Aug 18, 2020

dsentinel Aug 18, 2020

dsentinel Aug 18, 2020

damhuonglan commented Aug 25, 2020 •

edited by hrotman-pic

Loading

hrotman-pic left a comment

hrotman-pic commented Sep 29, 2020

ascire-pic commented Sep 29, 2020

damhuonglan commented Sep 29, 2020

hrotman-pic left a comment

damhuonglan commented Oct 27, 2020

I393 data ingestion commands: add option -F for continue mini file from #426

I393 data ingestion commands: add option -F for continue mini file from #426

Conversation

damhuonglan commented Aug 7, 2020 • edited Loading

What does this PR do?

Checklist

dsentinel left a comment

Choose a reason for hiding this comment

dsentinel Aug 18, 2020

Choose a reason for hiding this comment

dsentinel Aug 18, 2020

Choose a reason for hiding this comment

dsentinel Aug 18, 2020

Choose a reason for hiding this comment

damhuonglan commented Aug 25, 2020 • edited by hrotman-pic Loading

hrotman-pic left a comment

Choose a reason for hiding this comment

hrotman-pic commented Sep 29, 2020

ascire-pic commented Sep 29, 2020

damhuonglan commented Sep 29, 2020

hrotman-pic left a comment

Choose a reason for hiding this comment

damhuonglan commented Oct 27, 2020

damhuonglan commented Aug 7, 2020 •

edited

Loading

damhuonglan commented Aug 25, 2020 •

edited by hrotman-pic

Loading