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 new tests for (init functionality) #188

Closed
5 tasks
sbillinge opened this issue Dec 2, 2024 · 10 comments
Closed
5 tasks

write new tests for (init functionality) #188

sbillinge opened this issue Dec 2, 2024 · 10 comments

Comments

@sbillinge
Copy link
Contributor

sbillinge commented Dec 2, 2024

Problem

Proposed solution

@sbillinge sbillinge added this to the 3.6.0 release milestone Dec 2, 2024
@bobleesj
Copy link
Contributor

bobleesj commented Dec 4, 2024

@sbillinge just for clarification

In order to to use one of the methods (_set_angles_from_list) within init, would you like to have an extra parameter from

class DiffractionObject:
    def __init__(
        self, name=None, ..., xarray=None, yarray=None, xtype=None
    ):

to

class DiffractionObject:
    def __init__(
        self, name=None, ..., xarray=None, yarray=None, angles_list=None
    ):

    ....

    if angles_list:
        self._set_angles_from_list(angles_list)

in order to optionally use the _set_angles_from_list method?

    def _set_angles_from_list(self, angles_list):
        self.angles = angles_list
        self.n_steps = len(angles_list) - 1.0
        self.begin_angle = self.angles[0]
        self.end_angle = self.angles[-1]

@yucongalicechen
Copy link
Contributor

@sbillinge just for clarification

In order to to use one of the methods (_set_angles_from_list) within init, would you like to have an extra parameter from

class DiffractionObject:
    def __init__(
        self, name=None, ..., xarray=None, yarray=None, xtype=None
    ):

to

class DiffractionObject:
    def __init__(
        self, name=None, ..., xarray=None, yarray=None, angles_list=None
    ):

    ....

    if angles_list:
        self._set_angles_from_list(angles_list)

in order to optionally use the _set_angles_from_list method?

    def _set_angles_from_list(self, angles_list):
        self.angles = angles_list
        self.n_steps = len(angles_list) - 1.0
        self.begin_angle = self.angles[0]
        self.end_angle = self.angles[-1]

I think the angles here is on_tth(). See discussions in #191

@bobleesj
Copy link
Contributor

bobleesj commented Dec 4, 2024

@yucongalicechen please keep me posted. I will work on this issue once that PR is resolved.

@sbillinge
Copy link
Contributor Author

yes, @bobleesj , the UC is

  1. crystallographer wants to generate an x-array on a grid
  2. crystallogrpher (cr) inputs, start, stop, step, xtype
  3. DO creates xtype-array on this grid, and populates the other x-grids automatically.

I am not sure how this will be used tbh because there needs to be a yarray and this needs a matching xarray, so maybe we don't need this. But if there is already a yarray and the person wants everything on a different grid, a UC would be

  1. cr instantiates a DO with and xarray and a yarray and an xtype
  2. cr wants it on a different xarray
  3. cr supplies start, stop, step xtype
  4. DO generates new xarrays on this grid
  5. DO interpolates the yarray onto this grid and replaces the old yarray with this one

Again, I wonder actually how useful this would be. Maybe it is better to put it into an issue and leave it in case someone asks for it. The same would go for other ways to generate arrays.

@bobleesj
Copy link
Contributor

bobleesj commented Dec 6, 2024

@sbillinge Re-visiting this again,

is it a common practice for cr to modify/interpolate xarrays/yarrays?

If so, maybe the init wouldn't the best place but rather a separate public method that resets xarrays/yarrays and _all_arrays internally?

I could work on other issues if we decide not to have this for 3.6.0 release.

@bobleesj
Copy link
Contributor

bobleesj commented Dec 6, 2024

@sbillinge I've created an issue above.

Could we close this issue since you've provided init tests?

@yucongalicechen
Copy link
Contributor

@bobleesj Something new to add in insert_scattering_quantity: (1) check xarray empty or not, if not check that yarray is the same length as xarray and xtype is valid (one of XQUANTITIES), (2) set attribute so that we don't allow setting an invalid xtype.

@bobleesj
Copy link
Contributor

bobleesj commented Dec 6, 2024

@bobleesj Something new to add in insert_scattering_quantity: (1) check xarray empty or not, if not check that yarray is the same length as xarray and xtype is valid (one of XQUANTITIES), (2) set attribute so that we don't allow setting an invalid xtype.

When you say add sth new, are we referring to adding new tests or a new feature within the method? If it is a new feature within the method, i think its better to have a separate issue for this?

@yucongalicechen
Copy link
Contributor

yucongalicechen commented Dec 6, 2024

@bobleesj Something new to add in insert_scattering_quantity: (1) check xarray empty or not, if not check that yarray is the same length as xarray and xtype is valid (one of XQUANTITIES), (2) set attribute so that we don't allow setting an invalid xtype.

When you say add sth new, are we referring to adding new tests or a new feature within the method? If it is a new feature within the method, i think its better to have a separate issue for this?

add new features, sorry, and yes I will make a new issue for this (added now in #200)

@sbillinge
Copy link
Contributor Author

closed as completed in #193

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

No branches or pull requests

3 participants