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

Formal integral tests needed for interpolate_shells > 0 #1465

Closed
chvogl opened this issue Feb 19, 2021 · 9 comments · Fixed by #1499
Closed

Formal integral tests needed for interpolate_shells > 0 #1465

chvogl opened this issue Feb 19, 2021 · 9 comments · Fixed by #1499

Comments

@chvogl
Copy link
Contributor

chvogl commented Feb 19, 2021

As demonstrated through the bug from issue #1463, we need tests of the formal integral that also cover the case where "interpolate_shells > 0".

@chvogl chvogl changed the title Formal integral tests needed for interpolate_shells: > 0 Formal integral tests needed for interpolate_shells > 0 Feb 19, 2021
@antreev-brar
Copy link
Contributor

antreev-brar commented Feb 28, 2021

@chvogl I would like to work on this issue. Can you provide more details on how I can write these tests or some code that can be used as a reference.

@chvogl
Copy link
Contributor Author

chvogl commented Mar 3, 2021

It looks like I did not do my research on this one. We actually have tests for interpolate_shells > 0:
https://github.com/tardis-sn/tardis/blob/master/tardis/tests/test_tardis_full_formal_integral.py
The tests have been disabled by @wkerzendorf a few months ago with pytestmark = pytest.mark.skip(reason='memory problem') in the change from C to numba for the Monte Carlo radiative transfer calculations. To get it working again, you would have to remove the pytest.mark.skip and update the reference data for the test.

@antreev-brar
Copy link
Contributor

@chvogl So should I make a PR with these changes ( removing the pytest.mark.skip and updating reference data for test). Otherwise from my point of view, if there is no need for any PR then this issue must be closed.

@chvogl
Copy link
Contributor Author

chvogl commented Mar 5, 2021

@antreev-brar Go for it!

@aribalam
Copy link
Contributor

@chvogl what is the status of the issue?

@chvogl
Copy link
Contributor Author

chvogl commented Mar 15, 2021

@aribalam If you want to work on the issue, that would be great! You can read up on how to update the reference data on our documentation (although it might not be completely up to date).

@aribalam
Copy link
Contributor

@chvogl While updating the refence data, the integral test were failing due to the following error -

            simulation.runner.hdf_properties = [
                "j_blue_estimator",
                "spectrum",
                "spectrum_integrated",
            ]
>           simulation.runner.to_hdf(tardis_ref_data, "", self.name)

tardis/tests/test_tardis_full_formal_integral.py:67: 
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 
tardis/io/util.py:319: in to_hdf
    self.to_hdf_util(file_path_or_buf, buff_path, data, overwrite)
_ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ _ 

path_or_buf = <class 'pandas.io.pytables.HDFStore'>
File path: /media/arib/D/tardis/tardis-refdata/unit_test_data.h5

path = 'test_runner_simple_integral_macroatom'
elements = {'j_blue_estimator': array([[0., 0., 0., ..., 0., 0., 0.],
       [0., 0., 0., ..., 0., 0., 0.],
       [0., 0., 0., ....object at 0x7f9549eb7490>, 'spectrum_integrated': <tardis.montecarlo.spectrum.TARDISSpectrum object at 0x7f95575bc220>}
overwrite = False, complevel = 9, complib = 'blosc'

    @staticmethod
    def to_hdf_util(
        path_or_buf, path, elements, overwrite, complevel=9, complib="blosc"
    ):
        """
        A function to uniformly store TARDIS data to an HDF file.
    
        Scalars will be stored in a Series under path/scalars
        1D arrays will be stored under path/property_name as distinct Series
        2D arrays will be stored under path/property_name as distinct DataFrames
    
        Units will be stored as their CGS value
    
        Parameters
        ----------
        path_or_buf : str or pandas.io.pytables.HDFStore
            Path or buffer to the HDF file
        path : str
            Path inside the HDF file to store the `elements`
        elements : dict
            A dict of property names and their values to be
            stored.
        overwrite : bool
            If the HDF file path already exists, whether to overwrite it or not
    
        Notes
        -----
        `overwrite` option doesn't have any effect when `path_or_buf` is an
        HDFStore because the user decides on the mode in which they have
        opened the HDFStore ('r', 'w' or 'a').
        """
        try:  # when path_or_buf is a str, the HDFStore should get created
            buf = pd.HDFStore(path_or_buf, complevel=complevel, complib=complib)
        except TypeError as e:
            if e.message == "Expected bytes, got HDFStore":
                # when path_or_buf is an HDFStore buffer instead
                buf = path_or_buf
            else:
                raise e
        else:  # path_or_buf was a str
            if os.path.exists(path_or_buf) and not overwrite:
                buf.close()
>               raise FileExistsError(
                    "The specified HDF file already exists. If you still want "
                    "to overwrite it, set option overwrite=True"
                )
E               FileExistsError: The specified HDF file already exists. If you still want to overwrite it, set option overwrite=True

Any reason why this must be happening?

@chvogl
Copy link
Contributor Author

chvogl commented Mar 19, 2021

At first glance, this seems to be related to #1452. Maybe @Rodot- or @jaladh-singhal can comment on this.

@aribalam
Copy link
Contributor

@chvogl On careful observation, I am guessing the issue lies in not passing the overwrite=True keyword in this line. All other tests that write an HDF file in the reference data path has set this parameter to true.
On testing this locally, the test cases are passing. I will commit the changes if it seems alright to you.

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

Successfully merging a pull request may close this issue.

3 participants