Skip to content
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.

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

New yeoda filename #21

Merged
merged 59 commits into from
Oct 28, 2020
Merged

New yeoda filename #21

merged 59 commits into from
Oct 28, 2020

Conversation

fl0roth
Copy link
Contributor

@fl0roth fl0roth commented Oct 7, 2020

Implementation of the new filename and folder logic with a more compact and general purpose use. The corresponding changes include:

  • Introduction of a yeoda_naming class which represents the new filenaming and folder logic and a corresponding test_yeoda_naming routine.
  • Introduction of fields with variable length (defined by length = 0) in the SmartFilename and SmartFilenamePart class.
  • Introduction of an option (input parameter "compact") to use a compact filename design. If it is set to True, empty fields are represented by a single pad character and all fields are treated with variable length.
  • Adaption of the attached conda environment by updating the python-dateuntil modul (issue#18).

bbauerma and others added 30 commits September 23, 2020 18:06
…nd set the date format within this methods. The relative orbit is written into the extra_field which accepts more general inputs. Adapted the decoding and encoding methods according to this new field definition.
…een date and datetime using the isinstance function in the right order. Adapted the encoding method of the datetime field by checking for the 'T' to get time information. Removed deconding and encoding methods for time.
… statement to catch the corresponding inputs.
…s a variable length field at the end of it. Implemented rule which requires a delimiter in case of a variable length field.
… length to the __init__ function of the SmartFilenamePart.
…in case of smaller arguments compared to the defined length. Additionally, all empty fields are represented by a single pad. Adapted SmartFilenamePart class to handle this functionality.
…d if the argument is smaller then the expected length.
…compact option is True. Added if statement in the __set_item__ method to get this functionality.
Copy link
Collaborator

@bbauerma bbauerma left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks fine for me.
a) yeoda-filename works as expected
b) all tests on my local Windows machine are green

Copy link

@FelixReuss FelixReuss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • works fine with S1 Sigma

Copy link
Contributor

@cnavacch cnavacch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks very good! Three changes still need to be done:

  1. Fixing Python version (@bbauerma @fl0roth ?)
  2. Change loop in _build_fn() (I will do this).
  3. Add compact=False in the __init__ of all other file naming conventions. This might me a nice feature to have for the existing file naming conventions (@fl0roth ).

conda_env.yml Outdated
@@ -2,9 +2,9 @@ name: geopathfinder
channels:
- defaults
dependencies:
- python=3.6
- python
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to restrict the Python version to e.g. python=3.7. Simply using python is too general.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

actually, can we remove python at all?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it should work for python 3. for all python 3 versions (3.6, 3.7,..)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would recommend >= 3.6

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The issue I see at the moment is that we use the same conda yml file for Travis. I prefer to have separate yml files for testing each Python version with Travis. This ensures that each Python version works/is supported and is always tested and makes it easier to reset package dependencies for a specific Python version. In the main yml file I agree with @sebhahn , there we could set python >= 3.6.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is already set to python >= 3.6 now
please proceed and implement the steps you suggested - i want to release a new version...

src/geopathfinder/file_naming.py Outdated Show resolved Hide resolved
cnavacch and others added 3 commits October 21, 2020 15:51
@fl0roth fl0roth requested a review from cnavacch October 22, 2020 07:15
Copy link
Contributor

@cnavacch cnavacch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, all changes are fine from my side!

@bbauerma bbauerma merged commit 6f4a86a into master Oct 28, 2020
@bbauerma bbauerma deleted the new_YeodaFilename branch October 28, 2020 17:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants