BUG: Removing code that removes field type in set_title()

[chummels]

PR Summary

Removing code that removes field type in set_title(). Not sure why this was ever here. It is not present for any other plot annotations.

This resolves Issue #5087 .

[neutrinoceros]

This looks like a breaking change to me, the existing logic looks very much intentional (albeit flawed). Other plot annotations don't need to display a field name so I'm not surprised that it's the only one doing this.

[chummels]

This annotation also does not display a field name. When you call annotate_title('you give it a title'), it doesn't just grab the fieldname to put in the title. As far as I can tell, self.plot_title is a dictionary containing a list of fields that are being profiled/phaseplotted, and all this function does is create an entry in that dictionary with the field as the key and the title as the value. All my change does is force the dictionary key to be the full fieldname tuple instead of just the fieldname string. And this seems to work.

[chummels]

I added a test for this. Previously PhasePlot didn't have any SPH-based tests. Now it does, and it fails for dev tip but succeeds for this PR. I don't fully understand why this doesn't raise errors for grid-based datasets, but it doesn't seem to.

[chummels]

pre-commit.ci autofix

[chrishavlin]

looks good to me, and i agree with @chummels it's not a breaking change. It's only changing the key that's used to store the desired plot title.

[chrishavlin]

oh it was intentionally making it an ambiguous field, that's fun.

[chummels]

Yeah, I don't really understand why this was here. But I figured it was in such a niche corner of the code that no one had run into it except me. I don't think people often use the set_title() functionality, let alone for profiles, and specifically with particle-based datasets.

[chrishavlin]

I'll give @neutrinoceros a chance to respond, but will plan to merge later today or tomorrow.

[neutrinoceros]

please give me the night ! I don't remember the details but I do want to have another look tomorrow

[chrishavlin]

please give me the night ! I don't remember the details but I do want to have another look tomorrow

Sure thing! I can also wait a bit longer than tomorrow if you need more time.

[neutrinoceros]

Ok I'm willing to accept the change as is, but we need to make sure that the image tests are run. As long as the baseline submodule isn't updated, CI is expected to fail. The fact that it's passing here indicates that they are actually not picked up at all.

[neutrinoceros]

Got it: it's because the module they were added to is explicitly ignored for pytest

yt/pyproject.toml

Line 384 in 712b724

--ignore-glob='/*/yt/visualization/tests/test_profile_plots.py'

we need to move the test to a different file and make sure the tests are failing before we can move to the next step, as documented here
https://yt-project.org/docs/dev/developing/testing.html#creating-and-updating-image-baselines-for-pytest-mpl-tests

[chummels]

Oh. I didn't realize there were tests that we had which were being explicitly ignored. Why is that? It seems like not a great solution to just not be testing the profiler tests for bugs.

To what test file would you like me to move these tests?

[neutrinoceros]

Oh. I didn't realize there were tests that we had which were being explicitly ignored. Why is that? It seems like not a great solution to just not be testing the profiler tests for bugs.

That's because we're stuck in the middle of a migration from two test frameworks (nose to pytest): pytest doesn't support yield-based tests and nose is not maintained anymore (and broken on all versions of Python we still support). We have many tests that can only run on one runner, but we haven't collectively invested the time needed to get rid of nosetest altogether.

To what test file would you like me to move these tests?

yt/visualization/tests/test_image_comp_2D_plots.py would be a nice home to these

[chummels]

OK, so I moved the tests to the other file. Hopefully this is ready to go given that it's a 2-line fix for a simple bug. Answer tests aren't even really needed--just a unit test. I just copy-pasted the test from a non-SPH Profile test that included set_title().

[neutrinoceros]

Indeed, image test are now properly failing. Can you please make sure that the results match your expectations, and if so, open a PR to https://github.com/yt-project/yt_pytest_mpl_baseline, as documented ?

[neutrinoceros]

Actually I just noticed that the failure mode isn't quite right: it should be failing with "missing baseline", but it's showing changes in exisiting tests instead. This is because you apparently reused test names, so the image files are colliding with previous ones. Can you make sure you're using unique test names please ?

[neutrinoceros]

Looking at the images more closely, it is indeed not needed to add that many tests, in particular because none of them seems to actually check the problem you are fixing here. A single, targeted image test should be enough, but we do need some test for this bug, IMO.

[chrishavlin]

in particular because none of them seems to actually check the problem you are fixing here. A single, targeted image test should be enough, but we do need some test for this bug, IMO.

there's a new test_phaseplot_set_title that would cover it, no? And I think the motivation for the additional tests here is that there was no coverage at for the phaseplot functionality with particle data so I think it's helpful to add a bit of extra coverage beyond the fix for the actual bug here. Or are you saying you'd prefer those tests be added as unit tests just to make sure the cases run rather than image comparison tests?

[neutrinoceros]

there's a new test_phaseplot_set_title that would cover it, no?

it doesn't. It's using "Test title", not a tuple. It shouldn't be hard to change it to something useful though.

[chrishavlin]

the bug wasn't with set_title(tuple), it was when the phaseplot included an ambiguous field (#5087):

import yt
ds = yt.load_sample('FIRE_M12i_ref11')
p = yt.PhasePlot(ds, ('gas', 'density'), ('gas', 'temperature'), [('gas', 'z')], weight_field=None)
p.annotate_title('test')
p.save()

[chrishavlin]

sorry, meant to explain more but got distracted --

in

p = yt.PhasePlot(ds, ('gas', 'density'), ('gas', 'temperature'), [('gas', 'z')], weight_field=None)
p.annotate_title('test')

that ('gas', 'z') becomes just z in the annotate_title on main becuase it was removing the field type within annotate_title. this PR doesn't touch the actual label being used ('test').

e.g., here's the result of running the new test on main:

from yt.testing import fake_random_sph_ds
import yt

ds = fake_random_sph_ds(1000, [[0., 1.], [0., 1.], [0., 1]])
p = yt.PhasePlot(
            ds, ("gas", "density"), ("gas", "density"), ("gas", "mass")
        )   
p.annotate_title("Test Title")
p.render()

yt : [INFO     ] 2025-03-04 10:18:28,564 Parameters: current_time              = 0.0
yt : [INFO     ] 2025-03-04 10:18:28,564 Parameters: domain_dimensions         = [1 1 1]
yt : [INFO     ] 2025-03-04 10:18:28,564 Parameters: domain_left_edge          = [0. 0. 0.]
yt : [INFO     ] 2025-03-04 10:18:28,565 Parameters: domain_right_edge         = [1. 1. 1.]
yt : [INFO     ] 2025-03-04 10:18:28,565 Parameters: cosmological_simulation   = 0
yt : [INFO     ] 2025-03-04 10:18:28,565 Allocating for 1000 particles
Traceback (most recent call last):
  File "/Users/chavlin/src/temp/tempthing.py", line 8, in <module>
    p.annotate_title("Test Title")
  File "/Users/chavlin/src/yt_/yt_dev/yt/yt/visualization/_commons.py", line 112, in newfunc
    retv = f(self, *args, **kwargs)
  File "/Users/chavlin/src/yt_/yt_dev/yt/yt/visualization/profile_plotter.py", line 1324, in annotate_title
    self.plot_title[self.data_source._determine_fields(f)[0]] = title
  File "/Users/chavlin/src/yt_/yt_dev/yt/yt/data_objects/data_containers.py", line 1471, in _determine_fields
    finfo = self.ds._get_field_info(field)
  File "/Users/chavlin/src/yt_/yt_dev/yt/yt/data_objects/static_output.py", line 1001, in _get_field_info
    raise ValueError(
ValueError: The requested field name 'mass' is ambiguous and corresponds to any one of the following field types:
 ['gas', 'io', 'all']
Please specify the requested field as an explicit tuple (<ftype>, <fname>).

(it doesn't matter what string is provided to annotate_title)

[neutrinoceros]

Ow, I see. Sorry I didn't get it earlier. In that case, this one test should be sufficient. I have nothing against adding the other tests for coverage too, and ideally it should be a separate PR, but the process is probably cumbersome enough already so I don't mind if it all lives in this PR.

[chummels]

@chrishavlin is correct. I added the other tests just to ensure particle-based PhasePlots were getting tested for other annotations, just as grid-based PhasePlots are. I figured I might as well add them while I was updating things here, to potentially avoid other bugs down the line with more coverage. I didn't think image tests were necessary per se, since unit tests would catch any breaking failures, as they do with this particular bug. If I want these to just be unit tests, can I just remove the @pytest.mark.mpl_image_compare lines from the tests and then just get this merged?

I'll be honest that I am not really excited about doing all the steps to generate new image test answers for all of this. It's a two-line bugfix for a simple bug. So whatever I can do to expedite this and not use any more of anyone's time is what I'd prefer.

[chrishavlin]

If I want these to just be unit tests, can I just remove the @pytest.mark.mpl_image_compare lines from the tests

Yup, that's all you need to do to make them unit tests, but there'd be a couple of cleanup steps I'd recommend: you'd no longer need the test functions to return anything so you could strip out the return p.plots["gas", "mass"].figure lines and then I'd also suggest moving the tests to a different file again (sorry!!) since test_image_comp_2d_plots.py implies that all the contained tests are image comparisons. and depending on the results of #5129 I'd actually recommend moving them back to where you had them in the first place (again, sorry!!).