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

add fold! method #22

Merged
merged 2 commits into from
Jul 15, 2021
Merged

add fold! method #22

merged 2 commits into from
Jul 15, 2021

Conversation

joergbuchwald
Copy link
Contributor

@joergbuchwald joergbuchwald commented Jul 11, 2021

Add a folding method for the PlackettBurman design.

@phrb phrb self-requested a review July 11, 2021 00:13
@phrb
Copy link
Owner

phrb commented Jul 11, 2021

Looks good to me. The old travis setup no longer works, I'll just update it and then merge this. Thank you for the contribution!

@joergbuchwald
Copy link
Contributor Author

joergbuchwald commented Jul 11, 2021

Cool. I think, I will also work on porting some designs from pyDOE2 to julia soon, I started with BB already.

Copy link
Owner

@phrb phrb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  • Target only PlackettBurman designs, as done before, because the current implementation does not generalize for FactorialDesigns, for example (see discussion in comments below)
  • (up for discussion) Rename the method to mirror!, seems like a better verb than fold! for this purpose

Current method on a FactorialDesign:

julia> b = FullFactorial(([1, 2, 4], [:a, :b], [1.0, -1.0]))
FullFactorial
Dimension: (12, 3)
Factors: (factor1 = [1, 2, 4], factor2 = [:a, :b], factor3 = [1.0, -1.0])
Formula: 0 ~ factor1 + factor2 + factor3
Design Matrix:
12×3 DataFrame
│ Row │ factor1 │ factor2 │ factor3 │
│     │ Any     │ Any     │ Any     │
├─────┼─────────┼─────────┼─────────┤
│ 11       │ a       │ 1.0     │
│ 22       │ a       │ 1.0     │
│ 34       │ a       │ 1.0     │
│ 41       │ b       │ 1.0     │
│ 52       │ b       │ 1.0     │
│ 64       │ b       │ 1.0     │
│ 71       │ a       │ -1.0    │
│ 82       │ a       │ -1.0    │
│ 94       │ a       │ -1.0    │
│ 101       │ b       │ -1.0    │
│ 112       │ b       │ -1.0    │
│ 124       │ b       │ -1.0    │

julia> fold!(b)
24×3 DataFrame
│ Row │ factor1 │ factor2 │ factor3 │
│     │ Any     │ Any     │ Any     │
├─────┼─────────┼─────────┼─────────┤
│ 11       │ a       │ 1.0     │
│ 22       │ a       │ 1.0     │
│ 34       │ a       │ 1.0     │
│ 41       │ b       │ 1.0     │
│ 52       │ b       │ 1.0     │
│ 64       │ b       │ 1.0     │
│ 71       │ a       │ -1.0    │
│ 82       │ a       │ -1.0    │
│ 94       │ a       │ -1.0    │
│ 101       │ b       │ -1.0    │
│ 112       │ b       │ -1.0    │
│ 124       │ b       │ -1.0    │
│ 13-1      │ a       │ -1      │
│ 142       │ a       │ -1      │
│ 154       │ a       │ -1      │
│ 16-1      │ b       │ -1      │
│ 172       │ b       │ -1      │
│ 184       │ b       │ -1      │
│ 19-1      │ a       │ 1       │
│ 202       │ a       │ 1       │
│ 214       │ a       │ 1       │
│ 22-1      │ b       │ 1       │
│ 232       │ b       │ 1       │
│ 244       │ b       │ 1

src/design.jl Outdated Show resolved Hide resolved
src/design.jl Outdated Show resolved Hide resolved
src/design.jl Outdated

```
"""
function fold!(design::AbstractDesign)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the best solution would be to implement a mirror! method for PlackettBurman designs like you did before, and then adding specific methods of mirror! for different designs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The thing is, I was working already on 2-level fractional factorial designs (https://github.com/joergbuchwald/ExperimentalDesign.jl/tree/fractfact) and just saw that the same function could be used for fractional factorial design as well.
My suggestion would be to construct an abstract type that contains PlackettBurman as well as fractional factorial designs. What do you think?

Copy link
Owner

@phrb phrb Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's broader, but it would still be applicable only to 2-level designs with the current function.

I would prefer using multiple dispatch here to target different design types with different methods.

A "2-level abstract" type would also fix this, but I don't think it would be a good idea, because we can have fractional factorial designs with more than 2 levels, and they would be of different types.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We could also have a "fold.jl" file, with all implementations for different types.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is not wrong to call both screening designs.

Copy link
Owner

@phrb phrb Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good idea, I think. AbstractScreeningDesign would require extra subtypes such as AbstractMultilevelScreening though, otherwise we would get the same problem with this implementation being used on 3-level designs

Copy link
Owner

@phrb phrb Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

From an implementation point of view, how do you suggest we treat 2- and 3-+, and mixed-level designs differently?

Copy link
Contributor Author

@joergbuchwald joergbuchwald Jul 13, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Of course, theoretically, there can be a fractional factorial design with more than two levels, however, they are not very common.
(E.g. wikipedia states "In practice, one rarely encounters l > 2 levels in fractional factorial designs, since response surface methodology is a much more experimentally efficient way to determine the relationship between the experimental response and factors at multiple levels. In addition, the methodology to generate such designs for more than two levels is much more cumbersome.")
So thinking about implementation, I'm not sure whether it is worth to think about multilevel screening designs right now.
So if you think, it makes sense to consider them in the future, it would be enough from my perspective to create some abstract types for now.
Looking at other codes, it is quite common to distinguish between two-level and multilevel for factorial designs.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's a good plan. We can add a type for multi-level screening designs later if we need it.

Project.toml Outdated Show resolved Hide resolved
@phrb
Copy link
Owner

phrb commented Jul 15, 2021

@joergbuchwald I've updated tests and docs to use GitHub actions.
Could you please rebase on the current master so I can merge this?
Thanks again for contributing!

@phrb
Copy link
Owner

phrb commented Jul 15, 2021

I don't know if you can see the build report, but it's just a DataFrames syntax error:

ERROR: ArgumentError: syntax df[column] is not supported use df[!, column] instead
│    Stacktrace:
│     [1] getindex(#unused#::DataFrame, #unused#::String)
│       @ DataFrames ~/.julia/packages/DataFrames/vQokV/src/abstractdataframe/abstractdataframe.jl:2161
│     [2] fold!(design::PlackettBurman)
│       @ ExperimentalDesign ~/work/ExperimentalDesign.jl/ExperimentalDesign.jl/src/fold.jl:29
│     [3] top-level scope
│       @ none:1

@phrb
Copy link
Owner

phrb commented Jul 15, 2021

I'll merge this, it's just a difference due to the test seed. I'll regenerate the golden output with the right seed later.

@phrb phrb merged commit 9ad8e1e into phrb:master Jul 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants