statistikat / persephone

Object-oriented wrappers for RJDemetra
https://statistikat.github.io/persephone/
Other
7 stars 0 forks source link

Pure batch objects #15

Closed alexkowa closed 3 years ago

alexkowa commented 5 years ago

At the moment we don't have a class for batch objects, meaning multiple time series not within a hierachy. The main difference to the call xx <- per_hts(a=per_x13(AirPassengers), b=per_x13(AirPassengers)) might be that the Total should not be computed.

Is it worth to create such a class, maybe as an intermediate class between the single and the hierachical class?

GregorDeCillia commented 5 years ago

Yes, this should defintely be the next class to add to this package. I have some questions about the implementation details

Place in the polymorphic structure

The new batch class will be similar to per_hts() in the sense that both are able to contain multiple persephone objects. However, we can't reasonably define fields like output or adjusted because there is no "main timeseries" present in a batch. Therefore, I would define per_batch() as a standalone class that does not derive from persephone but accepts any number of persephone objects as components. This would mean the following calls are valid/invalid

# valid
per_batch(per_hts())

# invalid
per_hts(per_batch())

# invalid
per_batch(per_batch())

The third call might actually be reasonable but for now, I would not allow it.

alexkowa commented 5 years ago

Yes, I agree to your proposal what makes sense and what does not

merangelik commented 4 years ago

I am not sure if a default plot would make sense for per_batch(). Maybe a note/warning should be returned instead?

GregorDeCillia commented 4 years ago

I would not implement a dispatch for plot.per_batch(). Returning a warning or a note in such cases is possible, but quite unusual in the R language.

merangelik commented 3 years ago

Yes, this should defintely be the next class to add to this package. I have some questions about the implementation details

  • is the name per_batch() okay?
  • should it be possible to include per_hts() objects as components?
  • should the call to per_batch() create deep copies of its parameters? (Similar to #14)

Place in the polymorphic structure

The new batch class will be similar to per_hts() in the sense that both are able to contain multiple persephone objects. However, we can't reasonably define fields like output or adjusted because there is no "main timeseries" present in a batch. Therefore, I would define per_batch() as a standalone class that does not derive from persephone but accepts any number of persephone objects as components. This would mean the following calls are valid/invalid

# valid
per_batch(per_hts())

# invalid
per_hts(per_batch())

# invalid
per_batch(per_batch())

The third call might actually be reasonable but for now, I would not allow it.

@ is the name per_batch() okay?: I would go for per_mts() (multiple time series) as a complement to per_hts()

merangelik commented 3 years ago

So we decided that per_mts() might be associated with multivariate time series objects rather than multiple time series objects which makes per_batch() the better choice.

alexkowa commented 3 years ago

First version of per_batch is in the code. It can "run" , you can create a quality report and change parameters for all or specific parameters. @merangelik can you take a look?

alexkowa commented 3 years ago

@GregorDeCillia I am having some troubles with the "super" method inheritance thing. My logic now was that persephone-> per_batch->per_hts logic inherit from each other.

but you used super$ in some circumstances in per_hts which then references directly to persephone. I could not find something like super$super$... to use the method of two classes above.

alexkowa commented 3 years ago

Another idea:

GregorDeCillia commented 3 years ago

Yes, I think using private methods is the best course of action here

I suppose another way would be to privately expose the object super in per_batch

per_batch <- R6Class(
  inherit = "persephone",
  private = list(
    get_parent = function() { super }
  )
)

per_hts <- R6Class(
  inherit = "per_batch",
  list(
    some_method = function() { super$get_parent()$some_other_method() }
  )
)
alexkowa commented 3 years ago

Great that worked. I did the last method...