DAS-RCN / RCN_DASformat

4 stars 1 forks source link

Refactoring #12

Closed miili closed 1 year ago

miili commented 1 year ago
miili commented 1 year ago

@andreas-wuestefeld, @anowacki please have a look at the scaffolding and simplistic class for the event-based DAS data format.

browse here https://github.com/miili/RCN_DASformat/tree/refactor

andreas-wuestefeld commented 1 year ago

Thanks @miili for your extensive efforts This exceeds my python skills :-)

I still fully have to grasp the implications, and how to use this new version. maybe the README should include an example?

Data units should also allow for strain-rate, currently only strain seems to be allowed?

andreas-wuestefeld commented 1 year ago

@miili to be in line with variable name convetion, I guess "deltat" should be rather called "delta_t"

andreas-wuestefeld commented 1 year ago

@miili "start_time_ns" should probably be better explicitly an int64, what do you think

andreas-wuestefeld commented 1 year ago

in open() and is_valid(), the variable name "cls" should be more descriptive

andreas-wuestefeld commented 1 year ago

@miili maybe start_time and end_time should have a "_to_string" option so it is printed in human readable form?

miili commented 1 year ago

Hi Andreas, thanks for your comments. You can also start a Review and leave comments in-line and directly request changes from me.

I will make the changes you mentioned here.

Here are some information https://github.com/features/code-review

andreas-wuestefeld commented 1 year ago

installation with

pip install -e . does fail PS>pip install -e . ERROR: File "setup.py" or "setup.cfg" not found. Directory cannot be installed in editable mode: C:\Workspace\milii\RCN_DASformat (A "pyproject.toml" file was found, but editable mode currently requires a setuptools-based build.)

see also https://github.com/python-poetry/poetry/issues/34

Can you provide instructions on how to get the code running for testing?

miili commented 1 year ago

installation with

pip install -e . does fail see also python-poetry/poetry#34

Can you provide instructions on how to get the code running for testing?

Do you have an error message?

miili commented 1 year ago

@andreas-wuestefeld ping, requested changes are in. Please have a look and consider merging.