Closed Chilipp closed 5 years ago
I wonder if it isn't worth breaking this issue into three? To be completed in this order.
@justinalsing, just a suggestion based on personal experience:
Each of these can be non-trivial to do well, and all the moving parts can be overwhelming if you try to tackle them together.
Also be aware that CI build centric iterations can be much slower than running tests on your local machine. Generally the CI build of a branch involves all tests and is triggered by a commit that merges several changes from a scratch branch.... on your localhost you'll likely be running subsets of tests with regularity.
Hi @Chilipp @taqtiqa-mark
Following the discussion on the main review thread about the JOSS requirements, I’ve added a new notebook (dlm_validation_tests.ipynb
) with a suite of validation tests, for each of the DLM models included in the package. For each model, I (1) Generate mock data from the model (for user specified input parameters), (2) Run the DLM model on the mock data, (3) Plot the inferred model parameters against the inputs, showing that the inputs are recovered (within posterior uncertainties).
I’ve added a section to the README pointing the user to this notebook as a set of tests/validation for the full set of implemented models. I’ve also added comments in the CONTRIBUTING file, guiding contributors to include a similar test/demo of any new models implemented in this notebook.
Regarding testing the MPI script, as I discussed on the main review thread I really included this to make non MPI-savvy users lives easier rather than as an integral/critical part of the package. But I’ve emphasized in the README that the MPI script can be tested by running it with certain parameters set so such that it runs very short MCMC chains (and hence runs through very quickly).
Look forward to hearing whether you think this is all OK!
Best, Justin
Thanks @justinalsing ! I just started and apparently there is a file missing: utils/dlm.py
# Import required modules
import pystan
import matplotlib.pyplot as plt
import numpy as np
import time
import scipy.interpolate as interpolate
import netCDF4
import pickle
import scipy.stats as stats
from utils.utils import *
from utils.dlm import *
%matplotlib inline
---------------------------------------------------------------------------
ModuleNotFoundError Traceback (most recent call last)
<ipython-input-1-890c5b3f6cfd> in <module>
9 import scipy.stats as stats
10 from utils.utils import *
---> 11 from utils.dlm import *
12 get_ipython().run_line_magic('matplotlib', 'inline')
ModuleNotFoundError: No module named 'utils.dlm'
@Chilipp ah, thanks! I forgot to add it - I just added it now and pushed so it should be there now (sorry for that!)
Thank you @justinalsing. Unfortunately it it seems like I cannot run the test 2 for dlm_vanilla_ar2 because of the following error. Can you confirm this error?
---------------------------------------------------------------------------
RuntimeError Traceback (most recent call last)
<ipython-input-4-c33d4d85b5cc> in <module>
64
65 # Ok, let's run it
---> 66 fit = dlm_model.sampling(data=input_data, iter=4000, warmup=1000, chains=1, init = [initial_state], verbose=True, pars=('sigma_trend', 'sigma_seas', 'sigma_AR', 'rhoAR1', 'rhoAR2', 'trend', 'slope', 'beta', 'seasonal', 'ar'))
67
68 # Plot the results:
~/miniconda/envs/dlmmc/lib/python3.7/site-packages/pystan/model.py in sampling(self, data, pars, chains, iter, warmup, thin, seed, init, sample_file, diagnostic_file, verbose, algorithm, control, n_jobs, **kwargs)
670
671 seed = pystan.misc._check_seed(seed)
--> 672 fit = self.fit_class(data, seed)
673
674 m_pars = fit._get_param_names()
stanfit4anon_model_3780e43ed9ee465806912a94b90d6e06_7690838862458284755.pyx in stanfit4anon_model_3780e43ed9ee465806912a94b90d6e06_7690838862458284755.StanFit4Model.__cinit__()
RuntimeError: Exception: variable does not exist; processing stage=data initialization; variable name=nx; base type=int (in 'unknown file name' at line 6)
Here is some information about the working environment:
Hi @Chilipp - sorry about that (I forgot to add the updated stan models on my last commit). I've added and pushed it, and tested end-to-end by re-cloning, re-compiling the models and running through the dlm validation notebook. Apologies for not doing this check beforehand - as far as I am aware it is fully functional now, just make sure you re-compile the models after you pull the latest, thanks!
Hi @justinalsing, I'll likely get time some evening to look at this in the coming week. Do I understand correctly that you've done something like:
Correct?
@taqtiqa-mark great, thanks! Yes that's correct
Dear @justinalsing,
sorry for being so slow in this. I ran through your tests and they run through. However, there was another issue with the filenames of your regressors in the notebooks. Please note that UNIX filesystems are case-sensitive, there either please change the filenames of your regressors or change them in the notebook.
As an example: your file name is regressors/ENSO_MEI_1950_201802.txt but in your notebook you use regressors/enso_mei_1950_201802.txt. The same happened in the dlm_tutorial notebook.
But I have to say that I like your tests and they are really useful to judge the quality of dlmmc.
Hi @Chilipp - not a problem at all, and thanks for the comment. I've now made the regressor filenames case consistent with the actual filenames in the notebook examples (and pushed the changes)
Thanks
CI builds are not a blocking issue for me.
Hi @justinalsing. Strictly speaking, this issue is not yet solved (although it does not affect the acceptance of https://github.com/openjournals/joss-reviews/issues/1157). But if you close it, I assume this means that you do not plan to add CI builds within the near future?
Hi @Chilipp - that's right, I don't envisage adding CI builds in the near future (although it is on my to do list)
hey @justinalsing! As part of the software review in https://github.com/openjournals/joss-reviews/issues/1157 I create this issue to discuss the installation of automated tests using Continuous integration services (such as Travis) (see my comment (https://github.com/openjournals/joss-reviews/issues/1157#issuecomment-455693322) and the one of @taqtiqa-mark (https://github.com/openjournals/joss-reviews/issues/1157#issuecomment-459463115).
In your README file you state that the tutorial serves as a test for your suite (see https://github.com/justinalsing/dlmmc/commit/ccf6b831de09f5a55a2b4ed4839c82b2df834756). Nevertheless, this is not an automated test and (in terms of transparency) you should use any of the available CI services that runs your tests on every commit (e.g. Travis CI). I know, this is tedious but all open source projects that you are relying on went through this process, so why shouldn't yours as well?
Minimal requirements for your automated tests are:
jupyter nbconvert --execute
on your notebooksAdditionally, citing https://github.com/openjournals/joss-reviews/issues/1157#issuecomment-459463115
[x] You should definitely think about tests to check the mathematical correctness of your models since this is the central part of your code.
Comparing to some reference R-project, etc. would be ideal, of course. Alternatively you could just use the trend of your tutorial dataset (here you know, that this one is correct) and implement a unittest that runs your four models and compares it to this reference trend. You are the expert of your software so it's up to you which metric and which accuracy you are expecting of your model (but nevertheless, please justify your choice).
[ ] Parallel test suite that exercises the parallel calculations. You could for example run your
dlm_lat_alt_mpi_run.py
script in your test suiteIf you address this issue in a commit or pull request, please include the link to this issue (https://github.com/justinalsing/dlmmc/issues/5) in the commit/PR-message. In that way, it will be easier for us to follow the implementation.