-
Notifications
You must be signed in to change notification settings - Fork 6
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
Cleanup Variables API #207
Cleanup Variables API #207
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## devel #207 +/- ##
==========================================
+ Coverage 69.64% 70.52% +0.88%
==========================================
Files 51 52 +1
Lines 2421 2446 +25
==========================================
+ Hits 1686 1725 +39
+ Misses 735 721 -14 ☔ View full report in Codecov by Sentry. |
@Maximilian-Stefan-Ernst I have also cleaned up the existing unit tests and added the tests to cover the vars/params API calls |
Thanks again for this great PR, and also for adding/restructuring tests. Sorry for taking so long to review. If the comments are resolved, this is ready to merge; I also agree with the terminology. |
replaced by observed_vars()
replaced by nvars()
and add default implementation samples(::SemObserved)
* it is unused * if ever rowwise access would be required, it could be done with eachrow(data) without allocation
for missing data pattern: nobserved_vars() -> nmeasured_vars(), obs_cov/obs_mean -> measured_cov/measured_mean
@Maximilian-Stefan-Ernst Thank you for the review. I think I've addressed your comments: params, vars and observed APIs are now exported, StenoGraph explicit dependency is removed from the tests, as well as the alternative check_acyclic() method. |
399845f
into
StructuralEquationModels:devel
Thanks, I will update julia-format... |
Another round of cherry-picks from #193 that cleans up the variables API, as discussed in #199:
vars()
/nvars()
methods to get the vector of SEM model variables/count of variables.observed_vars()
/nobserved_vars()
to get the vector of observed variables ordered as in observed data (nobserved_vars()
replacesn_man()
method)latent_vars()
/nlatent_vars()
to get the vector of latent variables, preserving their order invars()
get_data()
renamed tosamples()
,nsamples()
is the number of data points/samples/observations in the observed data (nsamples()
replacesn_obs()
method)SemMissingData
nmeasured_vars()
is the count of observed variables with measurements (within the given data pattern)types.jl
to the files, where the methods of a particular type are defined (new files are created to accommodate methods for the abstract classes, e.g.SemSpecification
)RAMMatrices
orParameterTable
, but also for the types that refer to SEM specification (SemImply
,AbstractSem
etc)get_colnames()
, orget_n_nodes()
removedPer #199 discussion, an alternative to observed/latent terms would be manifest/latent. Also, samples could be replaced with observations (keeping both
nobserved_vars()
andnobservations()
may lead to confusion), and measured/missing terms could be replaced with observed/missing. I really don't have a strong preference here, so if SEM stakeholders think manifest/latent + observed/missing is a better choice, I can update the PR.Also, I think
obs_cov
/obs_mean
have to be updated accordingly (observed_cov
/observed_mean
ormanifest_cov
/manifest_mean
).This PR should be really the last one that does not introduce improvements or new features, but it should help to make the improvements easier.