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

revisit the invalid test DiffractionObject #241

Closed
Tracked by #143
sbillinge opened this issue Dec 16, 2024 · 8 comments · Fixed by #284
Closed
Tracked by #143

revisit the invalid test DiffractionObject #241

sbillinge opened this issue Dec 16, 2024 · 8 comments · Fixed by #284
Assignees
Milestone

Comments

@sbillinge
Copy link
Contributor

Problem

A bunch of tests became invalid when we changed the signature, but I think they were testing behavior that we wanted to be valid

Proposed solution

Check what behavior we want to test.

  • If it is valid behavior, change the test to make it valid and move it back to valid test
  • If we want to test something invalid check it is needed. We only need to check invalid behavior if we are handling it in some way.
@sbillinge sbillinge added this to the 3.6.0 release milestone Dec 16, 2024
@bobleesj bobleesj self-assigned this Dec 16, 2024
@bobleesj
Copy link
Contributor

@sbillinge I am trying to resolve this issue - the "signature" here refers to the 4 required init parameters for DiffractionObject?

I am trying to understand which test cases might have been invalid

@sbillinge
Copy link
Contributor Author

I had a quick look and I couldn't see any invalid tests that should be valid, so this may already be resolved.

When I wrote the comment it seemed that there were some tests failing that were moved to "invalid" rather than fixed so they passed....they had divide by zero etc.,. If we can find the PR number where the original comment was made I can quckly look and make sure it is resolved correctly.

@bobleesj
Copy link
Contributor

f we can find the PR number where the original comment was made I can quckly look and make sure it is resolved correctly.

Found it! #228 (comment) gotta love github

@sbillinge
Copy link
Contributor Author

OK, these are the guys in question:

tc_params = [
    (
        {},
        {
            "_all_arrays": np.empty(shape=(0, 4)),  # instantiate empty
            "metadata": {},
            "_input_xtype": "",
            "name": "",
            "scat_quantity": None,
            "qmin": np.float64(np.inf),
            "qmax": np.float64(0.0),
            "tthmin": np.float64(np.inf),
            "tthmax": np.float64(0.0),
            "dmin": np.float64(np.inf),
            "dmax": np.float64(0.0),
            "wavelength": None,
        },
    ),
    (  # instantiate just non-array attributes
        {"name": "test", "scat_quantity": "x-ray", "metadata": {"thing": "1", "another": "2"}},
        {
            "_all_arrays": np.empty(shape=(0, 4)),
            "metadata": {"thing": "1", "another": "2"},
            "_input_xtype": "",
            "name": "test",
            "scat_quantity": "x-ray",
            "qmin": np.float64(np.inf),
            "qmax": np.float64(0.0),
            "tthmin": np.float64(np.inf),
            "tthmax": np.float64(0.0),
            "dmin": np.float64(np.inf),
            "wavelength": None,
        },
    ),

@sbillinge
Copy link
Contributor Author

The first one is instantiating with empty arrays.

# C1: instantiate DO with empty arrays.  expect it to be a valid DO, but with everything empty

A potential UC for this might be
UC 1: load a DO from file

  1. User has a chi file on disc
  2. User wants it as a DO
  3. User instantiates an empty DO as mydo
  4. User runs mydo.load(filepath) which loads info into the DO.

We may not want to support this UC but provide a separate function outside the DO, though it kind of makes sense to have it this way too.

@sbillinge
Copy link
Contributor Author

the second one does

# C2 # instantiate just non-array attributes.  expect a valid DO with empty arrays, but some non array attributes set

The UC for this is
UC2: As UC 1 but metadata such as wavelength and xtype are not stored in the chi file

  1. as UC1.1 - 1.2
  2. User instantiates a DO with empty arrays but with wavelength and xtype info
  3. As UC1.4
  4. diffpy.utils load arrays from the file but does not overwrite existing metadata

We should think about exactly how to handle clashes, i.e., if the load finds the metadata in the file that is already loaded.

@sbillinge
Copy link
Contributor Author

We can put UC 1 and UC 2 into the 3.7.0 release. They are a bit tricky and related to other serialization issues.

But for now it means that we do want to be able to instantiate empty DO's I think, and so we want these tests to be valid.

I am not sure if they are valid and tested but perhaps @bobleesj you could check?

@bobleesj
Copy link
Contributor

thanks for this - it was a 10-15 min quick fix. Made a PR here! #284

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants