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

write tests for... #187

Closed
9 tasks
Tracked by #143
sbillinge opened this issue Dec 2, 2024 · 7 comments · Fixed by #293
Closed
9 tasks
Tracked by #143

write tests for... #187

sbillinge opened this issue Dec 2, 2024 · 7 comments · Fixed by #293
Assignees

Comments

@sbillinge
Copy link
Contributor

Problem

  • eq
  • add
  • radd
  • sub
  • rsub
  • mul
  • rmul
  • truediv
  • rtruediv

before doing this talk to @yucongalicechen or @sbillinge about new data structure for the arrays in the DOs.

Proposed solution

@bobleesj
Copy link
Contributor

bobleesj commented Dec 14, 2024

For the group's testing standards, please read #236 and #225 and #255

In the future, group's testing standards will be more thoroughly documented.

@bobleesj
Copy link
Contributor

@sbillinge I am trying to write tests for these. Two important questions below:

Q1. What's the difference between, for example mul and rmul shown below?

    def __mul__(self, other):
        multiplied = deepcopy(self)
        if isinstance(other, int) or isinstance(other, float) or isinstance(other, np.ndarray):
            multiplied.on_tth[1] = other * self.on_tth[1]
            multiplied.on_q[1] = other * self.on_q[1]
        elif not isinstance(other, DiffractionObject):
            raise TypeError("I only know how to multiply two Scattering_object objects")
        elif self.on_tth[0].all() != other.on_tth[0].all():
            raise RuntimeError(x_grid_emsg)
        else:
            multiplied.on_tth[1] = self.on_tth[1] * other.on_tth[1]
            multiplied.on_q[1] = self.on_q[1] * other.on_q[1]
        return multiplied

    def __rmul__(self, other):
        multiplied = deepcopy(self)
        if isinstance(other, int) or isinstance(other, float) or isinstance(other, np.ndarray):
            multiplied.on_tth[1] = other * self.on_tth[1]
            multiplied.on_q[1] = other * self.on_q[1]
        elif self.on_tth[0].all() != other.on_tth[0].all():
            raise RuntimeError(x_grid_emsg)
        else:
            multiplied.on_tth[1] = self.on_tth[1] * other.on_tth[1]
            multiplied.on_q[1] = self.on_q[1] * other.on_q[1]
        return multiplied

Q2. We've made all_arrays immutable - This makes the existing __mul__, __add__ methods invalid. We must create a new object. For example, the on_tth function above within __mul__ isn't valid since it can't modify all_arrays.

I am thinking we might have to change from

    def __add__(self, other):
        summed = deepcopy(self)
        if isinstance(other, int) or isinstance(other, float) or isinstance(other, np.ndarray):
            summed.on_tth[1] = self.on_tth[1] + other
            summed.on_q[1] = self.on_q[1] + other
        elif not isinstance(other, DiffractionObject):
            raise TypeError("I only know how to sum two DiffractionObject objects")
        elif self.on_tth[0].all() != other.on_tth[0].all():
            raise RuntimeError(x_grid_emsg)
        else:
            summed.on_tth[1] = self.on_tth[1] + other.on_tth[1]
            summed.on_q[1] = self.on_q[1] + other.on_q[1]
        return summed

to

    def __add__(self, other):
        if not isinstance(other, DiffractionObject):
            raise TypeError("I only know how to sum two DiffractionObject objects")
        tth_sum = self.on_tth[1] + other.on_tth[1]
        return DiffractionObject(xarray=tth_sum, yarray=self.yarray, xtype="tth", wavelength=self.wavelength)

The above assumes 1) self's yarray is used in the new object 2) tth xtype is used 3) and the self's wavelength is used.

@sbillinge
Copy link
Contributor Author

Q1. What's the difference between, for example mul and rmul shown below?

they are the same because the mul operation commutes.

@sbillinge
Copy link
Contributor Author

Q2. We've made all_arrays immutable - This makes the existing __mul__, __add__ methods invalid. We must create a new object. For example, the on_tth function above within __mul__ isn't valid since it can't modify all_arrays.

Those arrays are not immutable, we just made them not settable, right? I think we can update them how we want inside a method.

@bobleesj
Copy link
Contributor

Those arrays are not immutable, we just made them not settable,

Right, all_arrays isn't settable. I guess we can modify _all_arrays internally which is mutable.

@bobleesj
Copy link
Contributor

Since 3.6.0 deadline is approaching, I will resolve this issue

@bobleesj
Copy link
Contributor

bobleesj commented Dec 27, 2024

I will keep progress here:

Note to myself:

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 a pull request may close this issue.

2 participants