Skip to content

function lift_data and initialize_arrays#25

Merged
sbillinge merged 10 commits intodiffpy:mainfrom
aajayi-21:lift_data
Jul 31, 2023
Merged

function lift_data and initialize_arrays#25
sbillinge merged 10 commits intodiffpy:mainfrom
aajayi-21:lift_data

Conversation

@aajayi-21
Copy link
Copy Markdown
Contributor

No description provided.

@aajayi-21 aajayi-21 marked this pull request as ready for review July 24, 2023 18:51
Copy link
Copy Markdown
Contributor

@sbillinge sbillinge left a comment

Choose a reason for hiding this comment

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

please see comment.

Comment thread diffpy/snmf/subroutines.py Outdated

"""
data_input = np.asarray(data_input)
return data_input - (np.min(data_input[:]) * lift)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

doesn't data_input - (np.min(data_input) * lift) work? It is easier to read.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It does. I will change it

Comment thread diffpy/snmf/tests/test_subroutines.py Outdated
Comment thread diffpy/snmf/tests/test_subroutines.py Outdated
np.testing.assert_allclose(actual, expected, rtol=1e-03)


tld = [([[[1, -1, 1], [0, 0, 0], [2, 10, -3]], 1], [[4, 2, 4], [3, 3, 3], [5, 13, 0]]),
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

to make these easier to read, maybe make the inputs and outputs as tuples, something like:

tld = [
          (([[1, -1, 1], [0, 0, 0], [2, 10, -3]], 1), 
               ([4, 2, 4], [3, 3, 3], [5, 13, 0])),
          (([[1, -1, 1], [0, 0, 0], [2, 10, -3]], 0), 
                ([1, -1, 1], [0, 0, 0], [2, 10, -3])),

and so on.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I will do this

@sbillinge
Copy link
Copy Markdown
Contributor

@aajayi-21 a general comment I have is that it is very difficult for me to review this code because I don't have a kind of high level view of what all the pieces are, something that looks like a flow diagram that takes me from what is handed in at the beginning, through all the steps, to what is handed out. I feel that we could maybe come up with a better organization and data structures if we had this. Could you work on that? It can be hand-drawn with a pen and upload a photo to the issues or sthg like that.

@sbillinge sbillinge merged commit d526f5c into diffpy:main Jul 31, 2023
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.

2 participants