Skip to content

Conversation

@stevenhua0320
Copy link
Contributor

Closes #4 @sbillinge Ready to review

@codecov
Copy link

codecov bot commented Oct 25, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (f454574) to head (ac7ece7).
⚠️ Report is 18 commits behind head on main.

Additional details and impacted files
@@             Coverage Diff              @@
##             main       #34       +/-   ##
============================================
+ Coverage   50.00%   100.00%   +50.00%     
============================================
  Files           2         3        +1     
  Lines          18        45       +27     
============================================
+ Hits            9        45       +36     
+ Misses          9         0        -9     
Files with missing lines Coverage Δ
tests/conftest.py 100.00% <100.00%> (+64.28%) ⬆️
tests/test_load_image.py 100.00% <100.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
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.

@stevenhua0320 I can't see the tests for this. I guess that you started by writing a test, which is our workflow, right? :)

you can work with @zmx27 to get help with this, but

  1. there is a tif file in docs/examples/data (actually, that is where it should be)
  2. in conftest.py, copy this into the user_filesystem soemwhere
  3. use this fixture in test to try the different scenarios (directory is cwd, directory is absolute path, directory is relative path0

@stevenhua0320
Copy link
Contributor Author

@stevenhua0320 I can't see the tests for this. I guess that you started by writing a test, which is our workflow, right? :)

you can work with @zmx27 to get help with this, but

  1. there is a tif file in docs/examples/data (actually, that is where it should be)
  2. in conftest.py, copy this into the user_filesystem soemwhere
  3. use this fixture in test to try the different scenarios (directory is cwd, directory is absolute path, directory is relative path0

Actually I have tested with these scenarios on my local machine and they generated the correct matrix of that same tif file. But, you are right. It is more rigorous to write a test here.

@sbillinge
Copy link
Contributor

Actually I have tested with these scenarios on my local machine and they generated the correct matrix of that same tif file. But, you are right. It is more rigorous to write a test here.

yes, this is our workflow. Agree on the test. Only then write the function. The test captures the behavior we want (so in this case, three different successful cases). We can also think about how to handle unsuccessful cases. Probably give a helpful error message for FileNotFound or whatever. The error messages all have the form "what happened. What to do to fix it", so here it would be something like f"The file {file_path} cannot be found. Please rerun specifying a valid filename". There may also be an error reading the file, which would fail in a different way and require a different error message. We put "failures" in a different test.

@stevenhua0320
Copy link
Contributor Author

@sbillinge I'm designing for the test, right now it works well

Please rerun specifying a valid filename

Got the idea, in the original function, it has the FileNotFoundError and gives the error message. I'm still working on the good cases and also I have some ideas that how we gonna test the bad case. We probably can specify a file that is nowhere in the system (ex:nonexistent_file.tif) and let the function to find it. The desired matrix it should give is a matrix full of 0 as described in the original function. I'm still working on those test cases.

@sbillinge
Copy link
Contributor

that's the right idea.

We would usually build the desired user-filesystem as a fixture in conftest.py. In this way it can be reused across different tests. Take a look in diffpy.labpdfproc or diffpy.cmi to see examples.

Copy link
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.

Se comments

cwd_dir = base_dir / "cwd_dir"
cwd_dir.mkdir(parents=True, exist_ok=True)
test_dir = base_dir / "test_dir"
for d in (input_dir, home_dir, test_dir):
Copy link
Contributor

Choose a reason for hiding this comment

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

Don't use a single letter for the iterator. Always choose to make it more readable... E.g., for dir in (

I am not sure if you used an LLM for help. OK if you did, but try and work hard to bring the code up to group standards before pushing. It is really really tedious reviewing code from chatgpt....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, that is my habit of indexing object when writing for loops. I should stop this formatting and follow more readable format.

def example_tif():
"""Locate the example TIFF file relative to repo root."""
tif_path = Path("../docs/examples/KFe2As2-00838.tif").resolve()
if not tif_path.exists():
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooooh, don't be doing that. We want tests to fail of something is wrong

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I will delete this in the next PR.

return LoadImage(cfg)


# ----------------------------------------------------------------------
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove all these comment blocks

# LoadImage test setup
# ----------------------------------------------------------------------
@pytest.fixture(scope="module")
def example_tif():
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't need a fixture for htis. Just copy the file over as you build the filesystem above.

return tif_path


@pytest.fixture(scope="module")
Copy link
Contributor

Choose a reason for hiding this comment

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

i am not sure what this is doing but it doesn't seem appropriate to be building a fixture using one of hte functions that we will be testing. Just delete this.

As a heads-up, we don't do any testing in conftest.py we just build fixtures.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So, you mean we need to create a new test_loadimage.py and do all the tests there? If yes then I migrate the test to there.

return loader.loadImage(example_tif)


@pytest.fixture
Copy link
Contributor

Choose a reason for hiding this comment

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

see above, this should also be deleted.

# Unified test
# ----------------------------------------------------------------------


Copy link
Contributor

Choose a reason for hiding this comment

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

use pytest.mark.parametrize for the different cases. Please see examples in diffpy.cmi etc.

):
home_dir = user_filesystem["home"]
test_dir = user_filesystem["test"]
monkeypatch.setenv("HOME", str(home_dir))
Copy link
Contributor

Choose a reason for hiding this comment

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

we don't use monkeypatch but we use pytest mock if it is needed.

Copy link
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 comments.

@stevenhua0320
Copy link
Contributor Author

stevenhua0320 commented Oct 26, 2025

please see comments.

I have seen it and I'm currently making edits so that it would follow group's standard.
@sbillinge Really sorry professor I found I'm not so familiar with the use of how Pathlib actually works. It takes me a long time to think about the structure of the tree of files. Maybe still I need to refine the test if it is necessary. For the reason I can only make tags in the pytest.mark.parametrize is because in the case of absolute path, it must first go to its parent to mock to create the folder. That's why it cannot cluster the case of relative path and non-existance path together to make the same workflow of testing as far as I learned.

Copy link
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.

pls see comments

return image_loader.loadImage(tif_path)


def build_loadimage_path(case_tag, home_dir):
Copy link
Contributor

Choose a reason for hiding this comment

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

your test seems to have logic in it, but we don't really want that. the purpose of the tests is to encode behavior and then test logic in the function. The tests just have to capture behavior. please see below.



load_image_param = load_image_params = [
("abs", PROJECT_ROOT / "docs/examples/KFe2As2-00838.tif"),
Copy link
Contributor

Choose a reason for hiding this comment

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

this is where we capture the behavior. Please see the other projects for examples, but we want first a comment, e.g.

# case 1: just filename of file in current directory. expect function loads tiff file from cwd

then the paramaterize looks something like (input, output). In this case the input would be "KFe2As2-00838.tif" and the output would assert something that assures us that the file was correctly read.

Then similarly for case 2 # case 2: absolute file path to file in another directory. expect file is found and correctly read
and case 3 would be the relative path.

the test would look soething like

@pytest.mark.parametrize("input, expected")
def test_load_image_cases(input, expected, , user_filesystem):
    copy the test tiff to home_dir
    copy the test tiff to test_dir    # these two lines could be done in conftest.py if we want
    cd to home_dir
    actual = load_image(input)
    some asserts that compare actual and expected things.

@stevenhua0320
Copy link
Contributor Author

stevenhua0320 commented Oct 26, 2025

@sbillinge Professor Simon, If there is anything I could modify in the test file please give me more feedback and I would continue to change it.

Copy link
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.

the cases captured in the comments are now good, except Case 4 which I think should result in a crash, but the code needs some tlc. Please could you reach out to @zmx27 and work together on the implementation?

@stevenhua0320
Copy link
Contributor Author

stevenhua0320 commented Oct 26, 2025

the cases captured in the comments are now good, except Case 4 which I think should result in a crash, but the code needs some tlc. Please could you reach out to @zmx27 and work together on the implementation?

OK,actually in Case 4 in the implementation it would return the message like "file {filename} not found, please type in correct file name..." something like that and then return nothing. That's why in the test cases I check the datatype of what loadimage would return. I would reach out to Zhiming @zmx27 to see whether the implementation of loadimage needs further update.

@sbillinge
Copy link
Contributor

the cases captured in the comments are now good, except Case 4 which I think should result in a crash, but the code needs some tlc. Please could you reach out to @zmx27 and work together on the implementation?

OK,actually in Case 4 in the implementation it would return the message like "file {filename} not found, please type in correct file name..." something like that and then return nothing. That's why in the test cases I check the datatype of what loadimage would return. I would reach out to Zhiming @zmx27 to see whether the implementation of loadimage needs further update.

yes, I know that was your intent with case 4. I would rather that the users get that message but in a crash. In that case it would be in the test_function_bad. The next step is to make a clean PR from main with just the tests in it and no edits to loadimage. Then we can work on loadimage, but it is safer if we don't use any of the edits that were done in the past before the tests. We don't need to check the datatype of what loadimage returns I think, in this PR. EIther it can read a valid file or it can't and will crash.

Copy link
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.

Good progress. Still some things in the wrong direction.

We do not need ‎tests/cwd/example.tiff, ‎tests/home/example.tiff, or ‎tests/test/example.tiff so please delete these. We will make a clean PR later so we don't add and remove these files, but for now, please delete.



@pytest.mark.parametrize("input_path, expected", load_image_param)
def test_load_image_cases(input_path, expected, user_filesystem):
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be called test_load_image

)()
loader = LoadImage(cfg)

if expected:
Copy link
Contributor

Choose a reason for hiding this comment

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

don't have logic in your test

@pytest.mark.parametrize("input_path, expected", load_image_param)
def test_load_image_cases(input_path, expected, user_filesystem):
base_dir, home_dir, cwd_dir, test_dir = user_filesystem
test_file_dir = Path(__file__).parent
Copy link
Contributor

Choose a reason for hiding this comment

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

you seem to be working not in tmpdir, so don't be using test_file_dir in the tests. In fact don't even create this directory.

We will create all the directories we need in tmp_dir. Actually, we already have them, they are passed in from user_filesystem.

so we will move to cwd:

os.chdir(cwd_dir)

and we need to move our example file from docs/examples to the different places we will use it. So something like

source_dir = Path(__file__).parent / "docs" / "examples" / "data"
shutil.copy(source_dir / "example.tiff", test_dir) 
shutil.copy(source_dir / "example.tiff", .)

So we have an example tiff in cwd_dir (which is also the current working directory because we cd'd there) and also in ../test_dir.
After this we are fully working in the tempdir that will be built up before every test and removed after every test automatically.

Then we run as

actual_image = load_image(input_path)
assert something_about_actual_image == expected

we can decide different things to assert. That the shape is right, the total integrated intensity, and so on.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Professor, you are right. Now I learned how to deal with the conftest for user_system and now it would not create additional files in the step of copying as it is in a tmp_path. I also edited the testing here to both handle good case and bad case without using logic here. I would commit a new PR not introducing additional example.tiff in directory immediately.

Copy link
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.

Still a bit more to go. We are testing relative paths but not absolute. Also, please see comments

"Cfg", (), {"fliphorizontal": True, "flipvertical": False}
)()
loader = LoadImage(cfg)
actual = loader.loadImage(input_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

We want `loader.load_image() here

Copy link
Contributor Author

@stevenhua0320 stevenhua0320 Oct 29, 2025

Choose a reason for hiding this comment

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

You mean actual = loader.loadImage() instead of actual=loader.loadImage(input_path) ? But then the problem is how to determine which image's matrix is generated in the cwd ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't mean to change the signature, just to change the function name to load_image(...)

)()
loader = LoadImage(cfg)
actual = loader.loadImage(input_path)
assert isinstance(actual, np.ndarray)
Copy link
Contributor

Choose a reason for hiding this comment

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

This test seems a bit weak to me. Is there not more that we can test to check it worked ok?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or we can check its shape since it returns a np.ndarray here, as we copy the original file, it should be fine to check whether the shape is right. I think it is not feasible to directly check the content since it would return a 100 by 100 numpy matrix and I'm afraid we don't need to do that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

How about taking the expected = loader.loadImage(source_file) and actual = loader.loadImage(input_path)? The input_path is defined in the pytest.mark.parametrize. Then we check whether two arrays are equal. In the first three cases since it exists it would create the same matrix and the check would pass. The fourth case since the file is non-existant, before passed to assert sentence it will go into exception and give FileNotFoundError.

Copy link
Contributor

Choose a reason for hiding this comment

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

normally we wouldn't load an image, we just need some metrics from the test image and store them in the test, then load the image during the test and make sure we get hte same. the shape of the array is something, and a few other things such as the sum, the max the min, things like that.

# case 3: relative file path to file in another directory.
# expect file is found and correctly read
("./KFe2As2-00838.tif", True),
# case 4: non-existent file that incurred by mistype.
Copy link
Contributor

Choose a reason for hiding this comment

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

This test is incorrect. We want a crash for a min existing file

Copy link
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 comments.

**Fixed:**

* <news item>
* Fixed `loadimage` function in any directory.
Copy link
Contributor

Choose a reason for hiding this comment

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

load_image() function correctly finds files when passed a relative path

pic = np.array(pic[::-1, :])
return pic

def loadImage(self, filename):
Copy link
Contributor

Choose a reason for hiding this comment

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

let's make the function have a pep8 compliant name. We may as well fix these when we touch the function.

def loadImage(self, filename):
"""Load image file, if failed (for example loading an incomplete
file), then it will keep trying loading file for 5s.
"""Load image file using pathlib. If loading fails (e.g.
Copy link
Contributor

Choose a reason for hiding this comment

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

the use of pathlib is not relevant so don't have it in the docstring. You don't mention that you use numpy for arrays in the docstring, for instance.

if filenamefull.suffix == ".npy":
image = np.load(filenamefull)
else:
image = openImage(
Copy link
Contributor

Choose a reason for hiding this comment

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

if openImage is one of our functions, then let's modify it to accept a string or a Path object. Also, change it to be open_image()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I have checked that this function only make the use of trifffle.imread(...) function, which the arguement can be both a string and a Path object. Therefore, we do not need to modify the content of the function. We only need to modify the function name here to adhere PEP-8 standard.

except FileNotFoundError:
time.sleep(0.5)

image = self.flipImage(image)
Copy link
Contributor

Choose a reason for hiding this comment

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

let's change flipImage to flip_image() too while we are at it.

"Cfg", (), {"fliphorizontal": True, "flipvertical": False}
)()
loader = LoadImage(cfg)
actual = loader.loadImage(input_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't mean to change the signature, just to change the function name to load_image(...)

"Cfg", (), {"fliphorizontal": True, "flipvertical": False}
)()
loader = LoadImage(cfg)
actual = loader.loadImage(input_path)
Copy link
Contributor

Choose a reason for hiding this comment

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

change file name

Copy link
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.

pls see comments

)()
loader = LoadImage(cfg)
actual = loader.load_image(file_name)
expected = loader.load_image(source_file)
Copy link
Contributor

Choose a reason for hiding this comment

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

this logic is not right. You obviously can't use the function in the expected.

What is needed is that you make a scratch script an locally open the tiff file using some software that is not srxplanar, then extract some quantities from the resulting numpy array, like its shape, its max, min, mean, initial value and final value. Record these here in the test function as hard-coded numbers (put a comment telling future devs how you got them. Then run load_image and assert that what it returns have the same numbers.

Copy link
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.

pls see comments

# case 1: just filename of file in current directory.
# expect function loads tiff file from cwd
("KFe2As2-00838.tif", True),
("KFe2As2-00838.tif", np.float32(2595.7087)),
Copy link
Contributor

Choose a reason for hiding this comment

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

you don't have to pass these in in parametrize because they are always the same

actual = loader.load_image(file_name)
expected = loader.load_image(source_file)
assert np.array_equal(actual, expected)
actual = loader.load_image(file_name).mean()
Copy link
Contributor

Choose a reason for hiding this comment

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

delete .mean()

then you want

assert actual.shape() == expected_shape
assert actual.mean == expected_mean

and so on.

stevenhua0320 and others added 2 commits October 31, 2025 14:41
Updated expected values for image loading tests and corrected the source file path.
Copy link
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.

I edited the file to show what I had in mind. It is breaking the test now for sure but hopefully this gives you the idea.

@sbillinge
Copy link
Contributor

btw, I would like to get rid of the cfg clause too. We should decide what we want the function to do explicitly and then test that, so we don't want to put logic in the test that makes the function pass.

@stevenhua0320
Copy link
Contributor Author

btw, I would like to get rid of the cfg clause too. We should decide what we want the function to do explicitly and then test that, so we don't want to put logic in the test that makes the function pass.

Got the idea, I would integrate the hard-coded one for the first and last point of example tiff file, change it to the data directory, and try to eliminate the cfg clause and make another PR soon.

Copy link
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.

ok, good progress, we are getting there.

expected_first_point = 0
expected_last_point = 0

# locate source example file inside project docs
Copy link
Contributor

Choose a reason for hiding this comment

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

remove this comment.

shutil.copy(source_file, home_dir / "KFe2As2-00838.tif")

try:
loader = LoadImage(Mock(fliphorizontal=False, flipvertical=False))
Copy link
Contributor

Choose a reason for hiding this comment

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

is there a reason you are testing without using the defaults (so an empty argument)? this would make the test cleaner.

What program did you use to read the tiff file to get the expecteds?

With the defaults we would like the file to be read as it is, then the flipping vertical or horizontal will change the order of the elements in a way that flips the image horizontally or vertically. If you load the tiff file using imagej or some other program and plot it and then flip it, then get new values for various elements, then we can be sure the flipping logic is correct.

If we are testing different cases then send them in as part of the parametrize

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I used the imagej app to look at the tiff image and there is a image to result functionality to transform the image to matrix form. Should we also find a non-zero entry in the matrix and one we do nothing, the second we do flip_horizontal and the third we do flip_vertical. Se record the number for the same entry for the three scenarios in the expected and test whether the flipping logic is correct.

expected_mean = 2595.7087
expected_shape = (2048, 2048)
expected_first_point = 0
expected_last_point = 0
Copy link
Contributor

Choose a reason for hiding this comment

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

we need to pick different points if these are zero. We will use this for testing the flipping.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can we take the maximum/ minimum point here since they are invariant in any process. But I'm afraid that the minimum is also 0.

Copy link
Contributor

Choose a reason for hiding this comment

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

no, we don't want invariant points as we want to test the order. You can pick any points, e.g., [1986, 2019] or whatever, tbh as long as they have values in them and they will change when the image is flipped.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah I got your point and I should also write it in parametrization probably for the given entry value in the scenario.

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.

Cant load files not in the running directory

2 participants