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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Functional refactoring of save branch #29

Merged
merged 6 commits into from May 20, 2021
Merged

Functional refactoring of save branch #29

merged 6 commits into from May 20, 2021

Conversation

joaomcteixeira
Copy link
Member

@PicoCentauri
At the end I did not want to commit directly to the save branch. So I am doing a Pull Request to your branch 馃槃 馃惏 馃拪

I have refactored the save.py. Made the save_results kind of a pipeline approach. I think it reads great. There is no new algorithm. I have built on top of your implementations. This refactor will facilitate testing.

I am not finished yet, tests are still missing. But in this way you can comment on what I am doing.

@joaomcteixeira joaomcteixeira added the enhancement New feature or request label May 19, 2021
@joaomcteixeira joaomcteixeira added this to the Release v1 milestone May 19, 2021
@joaomcteixeira joaomcteixeira self-assigned this May 19, 2021
src/mdacli/save.py Outdated Show resolved Hide resolved
isinstance(arr, np.ndarray) \
and arr.ndim == 1

return valid


def stack_1d_arrays_list(list_1D, extra_list=None):
Copy link
Collaborator

Choose a reason for hiding this comment

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

One thing I would like to do when I wrote this function is not to restrict this to one list and an extra list but apply the operations on list_1D to a series of lists. The function definition could be something like

def stack_1d_arrays_list(list_1D, *args):
    """
    Stack a list of 1D numpy arrays of the same length vertically together.    

    The result is a list containing 2D arrays where each array got the same    
    number of rows. Yo can apply additional lists of 1d arrays as extra arguments. 
    The operations applied to the first are also applied to the 
    consecutive lists of arrays.

    Parameters    
    ----------    
    list_1d : list
       list of 1 dimensional numpy arrays

    ...
    """

Copy link
Member Author

Choose a reason for hiding this comment

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

I haven't changed that function. I am not sure yet about what you mean. I will look to it carefully later and "discord" with you. Anyway, we are meeting today at 11PM, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yes lets discuss this on discord. I can also take a look at this later.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we should address this on the save branch

Copy link
Collaborator

@PicoCentauri PicoCentauri left a comment

Choose a reason for hiding this comment

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

I like the new structure. Just one thing I had in mind: Why is there a save_1D_array and a save_results_array function? Can't these two be combined? Or at least the names are confusing. Which function should I use when I have an arbitrary array?

src/mdacli/save.py Show resolved Hide resolved
@joaomcteixeira
Copy link
Member Author

I like the new structure. Just one thing I had in mind: Why is there a save_1D_array and a save_results_array function? Can't these two be combined? Or at least the names are confusing. Which function should I use when I have an arbitrary array?

I will solve this. Nice comment +1

src/mdacli/cli.py Outdated Show resolved Hide resolved
joaomcteixeira and others added 3 commits May 20, 2021 23:23
before attempting mdacli saving engine.

Co-authored-by: Philip Loche <ploche@physik.fu-berlin.de>
correct json saving command
@joaomcteixeira
Copy link
Member Author

I think this is done.
If you like it we can merge to the save branch.

@joaomcteixeira
Copy link
Member Author

I like the new structure. Just one thing I had in mind: Why is there a save_1D_array and a save_results_array function? Can't these two be combined? Or at least the names are confusing. Which function should I use when I have an arbitrary array?

I addressed this.

@PicoCentauri PicoCentauri merged commit fc988a2 into save May 20, 2021
@PicoCentauri PicoCentauri deleted the saverefact branch November 18, 2021 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants