created a skeleton function opsim_time_series_images_data

[Nikki1510]

Here is my first draft at an opsim skeleton function. Many places still need to be filled out (marked with # To do), but I first want to check if the general idea is okay. I think it makes sense to create a separate function where OpSim Summary is initialised, because that takes a while. It's actually much faster if you have a list of sky coordinates to initialise those together, instead of doing it one coordinate at a time. We can talk about that on Monday.

To run it, you should have an OpSim database in the data/opsim_database folder, but that one was too large to upload to Github. And you should have a specific branch of OpSim Summary installed (Issue#325/proposalTables).

Let me know any input you have, I'm happy to change anything, this is only a rough first version :)

[Nikki1510]

It's failing the tests now because I defined some variables that I don't use yet, but that's still on the to-do list. What's the best way to get around this when writing a first code version?

[codecov]

Codecov Report

Attention: Patch coverage is 45.07042% with 39 lines in your changes missing coverage. Please review.

Project coverage is 90.23%. Comparing base (0a7c8f6) to head (d52d40e).
Report is 84 commits behind head on main.

Files with missing lines	Patch %	Lines
slsim/LsstSciencePipeline/opsim_pipeline.py	39.06%	39 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (0a7c8f6) and HEAD (d52d40e). Click for more details.

HEAD has 2 uploads less than BASE

Flag	BASE (0a7c8f6)	HEAD (d52d40e)
	4	2

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #125      +/-   ##
==========================================
- Coverage   97.90%   90.23%   -7.68%     
==========================================
  Files          70       72       +2     
  Lines        4687     5190     +503     
==========================================
+ Hits         4589     4683      +94     
- Misses         98      507     +409     

Files with missing lines	Coverage Δ
slsim/LsstSciencePipeline/lsst_science_pipeline.py	14.54% <ø> (ø)
slsim/Plots/plot_functions.py	95.91% <100.00%> (+0.36%)	⬆️
slsim/image_simulation.py	100.00% <100.00%> (ø)
slsim/LsstSciencePipeline/opsim_pipeline.py	39.06% <39.06%> (ø)

[ajshajib]

It's failing the tests now because I defined some variables that I don't use yet, but that's still on the to-do list. What's the best way to get around this when writing a first code version?

Hi @Nikki1510, the unit tests are passing, which is labeled as "CI / build (3.8)". So, there is no problem with the code itself. However, Pre-commit QA doesn't allow unused variables to maintain a high standard of code formatting. If this PR is still in progress, then presumably, you will use the variables in some future commit within this PR, and these errors will then clear up before the review and merging. However, if the variables are intended to be used in some code that would be provided in a later PR, you can comment out the variable definitions for now. A PR is expected to be one complete set of features, so if variables are meant to be used in a later PR, they should also be defined for the first time within that PR. This helps keep the codebase clean and will make reviewing the future PR easier. I hope this makes sense.

[Nikki1510]

@ajshajib Thank you, that makes a lot of sense! I've commented out the variables for now, so that the tests (hopefully) pass, but then this pull request should probably only be approved when I've expanded the function and used all the variables :)

[sibirrer]

Thank you very much @Nikki1510!
I have some requests for specifying in and output in the documentation. I leave it to @nkhadka21 about the code and if the routines should be split

[Nikki1510]

Hi all, I've updated the function with the PSF map (now a Moffat kernel assuming a power index of 3.1 and fwhm equal to the value from OpSim) and the zero point calculation (taken from OpSimSummary/opsimsummary/simlib.py/add_simlibCols).

[Nikki1510]

The output format is different from dp0_time_series_images_data because it has the sky brightness instead of dp0_time_series_cutout, and it also contains an entry bandpass to indicate in which band the observations are taken.

[nkhadka21]

Hi @Nikki1510, thank you very much for updating this PR! It looks good to me. It would be really helpful if you could upload a notebook showing use of your function. You can create a notebook similar to injection_of variable_lenses_in_dp0_time_series_images.ipynb .

[sibirrer]

Hi @Nikki1510 just checking in on this PR. I see that in the meantime there were some (minor) conflicts with the main branch. I hope these can be resolved on your end. Let us know if you need input and we are looking forward to having OpSim soon available to simulate light curves of SNe and quasars

[Nikki1510]

I tested the test functions on my laptop and they work 👍
But I don't think they work if the user doesn't have the exact same OpSim database installed... Not sure how to work around that to test the opsim_time_series_images_data function. Or could this be enabled as an optional test, only if you want to use OpSim?

[sibirrer]

Hi @Nikki1510, is your specific Opsim version on GitHub?
You could add it in test_requirements.txt with
git+https://github.com/nikki1510/opsim@main#egg=opsim (change the url and name in case)

[Nikki1510]

Hi @sibirrer, there are 2 different issues here: having OpSimSummaryV2 installed (which is on Github: https://github.com/LSSTDESC/OpSimSummaryV2). Do we want to add that to the requirements for everyone using SLSim?(for example, some people may only want to simulate static images and not go through the effort of installing OpSimSummaryV2).

The second question is about having downloaded the actual OpSim database and stored that in the right folder. That is not on Github but has to be downloaded from another website, and the files are quite large. Similarly, I don't know if we want to require that everyone has such a database downloaded. But if these things are not fulfilled then the test function for the opsim_time_series_images_data function will not work.

[sibirrer]

Hi @sibirrer, there are 2 different issues here: having OpSimSummaryV2 installed (which is on Github: https://github.com/LSSTDESC/OpSimSummaryV2). Do we want to add that to the requirements for everyone using SLSim?(for example, some people may only want to simulate static images and not go through the effort of installing OpSimSummaryV2).

if it's in the test_requirements.txt then it gets not installed by default in the installation, only for test purposes. I just hope the installation is not too long for the CI to run

[sibirrer]

The second question is about having downloaded the actual OpSim database and stored that in the right folder. That is not on Github but has to be downloaded from another website, and the files are quite large. Similarly, I don't know if we want to require that everyone has such a database downloaded. But if these things are not fulfilled then the test function for the opsim_time_series_images_data function will not work.

No, those files should not be required to be downloaded by the user. I would suggest as an input the user gives a path to these files, and if these files are not there (or provided), the code should raise an error with a test that state where the files need to be downloaded from.

[nkhadka21]

Hi @Nikki1510 , as Simon mentioned, I think putting OpsimsummaryV2 in test_requirements.txt will solve the first error. For the second error, it seems like the test function is unable to read your provided path for expo_data_opsim.hdf5 file. Once you provide a correct relative path, then that will solve this error. Thank you.

[sibirrer]

We have used directory paths and then relative to those, i.e.

path = os.path.dirname(file)
module_path, _ = os.path.split(path)
print(path, module_path)
blue_one = Table.read(
os.path.join(path, "TestData/blue_one_modified.fits"), format="fits"
)

[sibirrer]

wohoo, the python problem got resolved. Now it's a weird thing with coolest. @nkhadka21 can you have a look at the CI? Thank you!

[nkhadka21]

wohoo, the python problem got resolved. Now it's a weird thing with coolest. @nkhadka21 can you have a look at the CI? Thank you!

Hi @sibirrer and @Nikki1510, I made an update in slsim coolest interface. Hopefully, this update will solve this error. However, got the same precommit error as Nikki's PR.

[sibirrer]

@Nikki1510: @nkhadka21 merged a fix for the coolest part. So if you update your branch, it should hopefully work. We will ignore the pre-commit QA failure for now. Thank you!

[Nikki1510]

Hi @sibirrer and @nkhadka21, the CI build was successful now, but the pre-commits are still failing..

[sibirrer]

@Nikki1510 we will ignore the recommit-CI tests. We only look at the unit tests and test coverage. I let @nkhadka21 look into whether tests can be improved or not, knowing that we do not need to unit test the OpSim summary part

[nkhadka21]

Hi @Nikki1510 , thank you very much for your wonderful work! It looks good to me.