function construct_component_matrix #43

aajayi-21 · 2023-08-03T17:01:27Z

No description provided.

created docstring and tests for construct_component_matrix

Filled and marked test cases.

sbillinge

I think this already got merged. Did you update your main branch?

aajayi-21 · 2023-08-05T15:08:39Z

I believe the last merge added the function that creates the stretching factor matrix, not the component matrix

sbillinge

couple of comments.

sbillinge · 2023-08-05T15:33:33Z

diffpy/snmf/subroutines.py

@@ -84,6 +84,35 @@ def construct_stretching_matrix(components, number_of_components, number_of_sign
    return stretching_factor_matrix


+def construct_component_matrix(components, number_of_components, signal_length):


we don't need to pass in number_of_components because that is len(components). I fixed this in the other function before merging, but looking back I see that I did all the t hings except remove it from the signature. Please can you remove it from the arguments here and and also there? Unless I have missed something....

Ok. I will fix that here and in the other function

sbillinge · 2023-08-05T15:37:28Z

diffpy/snmf/subroutines.py

+    if (number_of_components <= 0) or (len(components) <= 0):
+        raise ValueError(f"Number of components = {number_of_components}. Number_of_components must be >= 1")
+
+    component_matrix = np.zeros((signal_length, number_of_components))


this is a slightly ugly way of doing it. Better to maybe use an append. Also, is there a reason we are not doing like

component_matrix[i] = component.iq

component_matrix is a 2d array so I think that you need two indices. I'm trying to replace each column with columbia.iq

This doesn't change that because you you are putting a vector at each position, i. It is just syntactically cleaner.

I see. I will change it

replaced with len(components). corrected tests to account

sbillinge · 2023-08-06T15:10:44Z

....of course you have to switch the order of the axes for this to work.....

sbillinge

still a bit more cleaning to do

sbillinge · 2023-08-07T11:44:30Z

diffpy/snmf/subroutines.py

@@ -105,10 +105,10 @@ def construct_component_matrix(components, signal_length):
    if len(components) <= 0:
        raise ValueError(f"Number of components = {len(components)}. Number_of_components must be >= 1")

-    component_matrix = np.zeros((signal_length, len(components)))
+    component_matrix = np.zeros((signal_length, len(components))).T


I think it would be clearer if you did component_matrix = np.zeros((len(components), signal_length)) and then returned component_matrix, no?

Yes. component_matrix would have the incorrect shape, so you would have to transpose it.

even after you change the order in the np.zeros as I did?

component_matrix should have the shape signal_length by number_of_components once it is returned. If I had component_matrix = np.zeros((len(components), signal_length)) I think that I would have to transpose it after I have replaced the rows inside the for loop.

sbillinge · 2023-08-07T11:45:10Z

diffpy/snmf/subroutines.py

+    """
+    if signal_length <= 0:
+        raise ValueError(f"Signal length = {signal_length}. Signal length must be >= 1")
+    if len(components) <= 0:


can a length be less than zero?

sbillinge · 2023-08-07T11:46:52Z

diffpy/snmf/subroutines.py

+      The matrix containing the component signal values. Has dimensions `signal_length` x `number_of_components`.
+
+    """
+    if signal_length <= 0:


isn't signal_length = len(components[0].iq)?

So you don't need to pass it in either.

That's true. I will change it

sbillinge

pls see comment

sbillinge · 2023-08-07T15:00:31Z

diffpy/snmf/subroutines.py

@@ -105,10 +105,10 @@ def construct_component_matrix(components, signal_length):
    if len(components) <= 0:
        raise ValueError(f"Number of components = {len(components)}. Number_of_components must be >= 1")

-    component_matrix = np.zeros((signal_length, len(components)))
+    component_matrix = np.zeros((signal_length, len(components))).T


even after you change the order in the np.zeros as I did?

sbillinge

small cleaning.

Also, is there a reason that you need the array to be transposed? I know it has to be that way for your code to work, but we could also update the code. If there is a reasonable mathematical reason let's keep it this way, but just check later and let me know. Is it because a matrix multiplication later ?

sbillinge · 2023-08-07T15:17:58Z

diffpy/snmf/tests/test_subroutines.py

@@ -188,7 +188,7 @@ def test_construct_stretching_matrix(tcso):
        #assert actual[component.id, :] == component.stretching_factors

 tccm = [ # (ComponentSignal([0,.25,.5,.75,1],20,0),1,5), # Expected to fail
-        ([ComponentSignal([0,.25,.5,.75,1],20,0)],5),
+        ([ComponentSignal([0,.25,.5,.75,1],20,0)]),
        # ([ComponentSignal([0,.25,.5,.75,1],20,2)],1,5), # Expected to fail


please can you remove or fix these commented out ones. Probably many of them are not needed now we are not passing in constants. But there may be some new ones you want to fail, but just 2 I can think of

aajayi-21 · 2023-08-07T16:03:10Z

I don't believe there is a reason that it needs to be transposed. The component matrix is directly used in matrix multiplication.

sbillinge · 2023-08-07T16:10:13Z

That's much cleaner and easier to read.... thanks so much. Very satisfying! This code will end up being much nicer and easier to maintain!

aajayi-21 added 3 commits August 3, 2023 12:00

initial commit

7618592

created docstring and tests for construct_component_matrix

created test cases

55bc8b2

filled construct_component_matrix

5f34b69

Filled and marked test cases.

aajayi-21 marked this pull request as ready for review August 3, 2023 17:30

sbillinge reviewed Aug 5, 2023

View reviewed changes

aajayi-21 added 2 commits August 6, 2023 09:56

removed number_of_components

77bb754

replaced with len(components). corrected tests to account

indexing for component_matrix

b92fbd9

switching axes order

83877ec

sbillinge reviewed Aug 7, 2023

View reviewed changes

removed signal_length parameter. changed axes of component_matrix

bc3556f

sbillinge reviewed Aug 7, 2023

View reviewed changes

removed transpose. removed/correct tests

50ea324

sbillinge merged commit 8cf7163 into diffpy:main Aug 7, 2023

		@@ -84,6 +84,35 @@ def construct_stretching_matrix(components, number_of_components, number_of_sign
		return stretching_factor_matrix


		def construct_component_matrix(components, number_of_components, signal_length):

function construct_component_matrix #43

function construct_component_matrix #43

Uh oh!

Conversation

aajayi-21 commented Aug 3, 2023

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

aajayi-21 commented Aug 5, 2023

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbillinge commented Aug 6, 2023

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbillinge left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aajayi-21 commented Aug 7, 2023

Uh oh!

sbillinge commented Aug 7, 2023

Uh oh!

Uh oh!