Cleanup Variables API #207

alyst · 2024-06-17T06:52:11Z

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() replaces n_man() method)
latent_vars()/nlatent_vars() to get the vector of latent variables, preserving their order in vars()
get_data() renamed to samples(), nsamples() is the number of data points/samples/observations in the observed data (nsamples() replaces n_obs() method)
for SemMissingData nmeasured_vars() is the count of observed variables with measurements (within the given data pattern)
interface method definitions are moved from 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)
these variable API methods should work not only for the SEM specification objects, like RAMMatrices or ParameterTable, but also for the types that refer to SEM specification (SemImply, AbstractSem etc)
old methods using ambiguous terms get_colnames(), or get_n_nodes() removed

Per #199 discussion, an alternative to observed/latent terms would be manifest/latent. Also, samples could be replaced with observations (keeping both nobserved_vars() and nobservations() 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 or manifest_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.

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

github-actions · 2024-06-17T06:53:25Z

src/frontend/fit/summary.jl

+        check_round(partable.columns[c][var_indices]; digits = digits) for c in var_columns
    )


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

check_round(partable.columns[c][var_indices]; digits = digits) for c in var_columns

)

check_round(partable.columns[c][var_indices]; digits = digits) for c in var_columns

github-actions · 2024-06-17T06:53:25Z

src/imply/RAM/generic.jl

@@ -143,9 +144,13 @@ function RAM(;
    # μ
    if meanstructure
        has_meanstructure = Val(true)


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

has_meanstructure = Val(true)

!isnothing(M_indices) || throw(

ArgumentError(

"You set `meanstructure = true`, but your model specification contains no mean parameters.",

),

)

codecov · 2024-06-17T06:59:03Z

Codecov Report

Attention: Patch coverage is 91.80328% with 15 lines in your changes missing coverage. Please review.

Project coverage is 70.65%. Comparing base (b2012b0) to head (cf50d8c).
Report is 1 commits behind head on devel.

Files	Patch %	Lines
src/frontend/specification/RAMMatrices.jl	71.42%	4 Missing ⚠️
src/observed/EM.jl	80.00%	4 Missing ⚠️
src/frontend/specification/StenoGraphs.jl	80.00%	2 Missing ⚠️
src/frontend/common.jl	80.00%	1 Missing ⚠️
src/frontend/fit/standard_errors/bootstrap.jl	0.00%	1 Missing ⚠️
src/frontend/fit/summary.jl	0.00%	1 Missing ⚠️
src/imply/RAM/generic.jl	93.33%	1 Missing ⚠️
src/imply/RAM/symbolic.jl	85.71%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##            devel     #207      +/-   ##
==========================================
+ Coverage   69.64%   70.65%   +1.01%     
==========================================
  Files          51       52       +1     
  Lines        2421     2457      +36     
==========================================
+ Hits         1686     1736      +50     
+ Misses        735      721      -14

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

for missing data pattern: nobserved_vars() -> nmeasured_vars(), obs_cov/obs_mean -> measured_cov/measured_mean

github-actions · 2024-06-26T20:53:26Z

src/frontend/fit/summary.jl

+        check_round(partable.columns[c][var_indices]; digits = digits) for c in var_columns
    )


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

check_round(partable.columns[c][var_indices]; digits = digits) for c in var_columns

)

check_round(partable.columns[c][var_indices]; digits = digits) for c in var_columns

github-actions · 2024-06-26T20:53:26Z

src/imply/RAM/generic.jl

@@ -143,9 +144,13 @@ function RAM(;
    # μ
    if meanstructure
        has_meanstructure = Val(true)


[JuliaFormatter] _{reported by reviewdog 🐶}

Suggested change

has_meanstructure = Val(true)

!isnothing(M_indices) || throw(

ArgumentError(

"You set `meanstructure = true`, but your model specification contains no mean parameters.",

),

)

alyst · 2024-06-27T07:57:43Z

@Maximilian-Stefan-Ernst I have also cleaned up the existing unit tests and added the tests to cover the vars/params API calls

Maximilian-Stefan-Ernst · 2024-07-12T11:26:57Z

src/frontend/common.jl

+"""
+    nparams(semobj)
+
+Return the number of parameters in a SEM model associated with `semboj`.


Suggested change

Return the number of parameters in a SEM model associated with `semboj`.

Return the number of parameters in a SEM model associated with `semobj`.

Maximilian-Stefan-Ernst · 2024-07-12T14:33:08Z

src/frontend/common.jl

+
+See also [`vars`](@ref).
+"""
+nvars(semobj) = length(vars(semobj))


This function (and the next ones) do not seem to be exported - I think we should do that.

alyst and others added 15 commits June 16, 2024 21:18

common.jl: common vars API methods

0645a4a

SemSpecification: vars API

8b4abcf

RAMMatrices: vars API

a90defc

ParamTable: vars API

74c4550

SemImply: vars and params API

800198c

RAM imply: use vars API

c7a58ec

RAMSymbolic: use vars API

534330d

start_simple(): use vars API

2be665e

starts_fabin3: use vars API

8cf1f24

remove get_colnames()

7c51f8f

replaced by observed_vars()

remove get_n_nodes()

5ed430b

replaced by nvars()

get_data() -> samples()

277f146

and add default implementation samples(::SemObserved)

SemObsData: remove rowwise

a197fa3

* it is unused * if ever rowwise access would be required, it could be done with eachrow(data) without allocation

AbstractSemSingle: vars API

0a15204

rename n_obs() -> nsamples()

6503a34

github-actions bot reviewed Jun 17, 2024

View reviewed changes

alyst and others added 5 commits June 26, 2024 13:51

rename n_man() -> nobserved_vars()

77a0cba

for missing data pattern: nobserved_vars() -> nmeasured_vars(), obs_cov/obs_mean -> measured_cov/measured_mean

move Sem methods out of types.jl

ffc43a0

rows(::SemObservedMissing) -> pattern_rows()

ab47745

fix formatting

c54fa33

samples(SemObsCov) throws an exception

ee5b0be

alyst force-pushed the vars_api branch from e57223b to d4cb759 Compare June 26, 2024 20:52

github-actions bot reviewed Jun 26, 2024

View reviewed changes

alyst added 6 commits June 27, 2024 00:13

SemObserved tests: refactor and add var API tests

91a149d

ParTable(graph): group is only valid for ensemble

1f1f976

ParTable(graph): fix NaN modif detection

1f12087

refactor SemSpec tests

6f3afa8

add Sem unit tests

2982774

test check_acyclic()

cf50d8c

alyst force-pushed the vars_api branch from 7994476 to cf50d8c Compare June 27, 2024 07:56

Maximilian-Stefan-Ernst reviewed Jul 12, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cleanup Variables API #207

Cleanup Variables API #207

alyst commented Jun 17, 2024

github-actions bot Jun 17, 2024

github-actions bot Jun 17, 2024

codecov bot commented Jun 17, 2024 •

edited

Loading

github-actions bot Jun 26, 2024

github-actions bot Jun 26, 2024

alyst commented Jun 27, 2024

Maximilian-Stefan-Ernst Jul 12, 2024

Maximilian-Stefan-Ernst Jul 12, 2024

		check_round(partable.columns[c][var_indices]; digits = digits) for c in var_columns
		)

-        has_meanstructure = Val(true)
+        !isnothing(M_indices) || throw(
+            ArgumentError(
+                "You set `meanstructure = true`, but your model specification contains no mean parameters.",
+            ),
+        )

	Return the number of parameters in a SEM model associated with `semboj`.
	Return the number of parameters in a SEM model associated with `semobj`.

Cleanup Variables API #207

Are you sure you want to change the base?

Cleanup Variables API #207

Conversation

alyst commented Jun 17, 2024

github-actions bot Jun 17, 2024

Choose a reason for hiding this comment

github-actions bot Jun 17, 2024

Choose a reason for hiding this comment

codecov bot commented Jun 17, 2024 • edited Loading

Codecov Report

github-actions bot Jun 26, 2024

Choose a reason for hiding this comment

github-actions bot Jun 26, 2024

Choose a reason for hiding this comment

alyst commented Jun 27, 2024

Maximilian-Stefan-Ernst Jul 12, 2024

Choose a reason for hiding this comment

Maximilian-Stefan-Ernst Jul 12, 2024

Choose a reason for hiding this comment

codecov bot commented Jun 17, 2024 •

edited

Loading