search

sizespectrum / mizer

Multi-species size-based ecological modelling in R
https://sizespectrum.org/mizer
37 stars 42 forks source link

Stop using S4 classes #56

Open gustavdelius opened 5 years ago

gustavdelius commented 5 years ago

Currently mizer uses S4 classes for the MizerParams and MizerSim objects, instead of the more usual S3 methods. S4 classes are more strictly enforcing the structure of the object. This has the unfortunate consequence that if a user saves a MizerParams or MizerSim object with one version of mizer, then it can not be loaded with a later version of mizer if in that version the MizerParams class has gained extra slots, for example. We can anticipate having to add extra slots in the future for new mizer features.

Slots in S3 objects are accessed via $ whereas slots in S4 objects are accessed via @. So currently we have syntax like params@species_params$w_mat because the MizerParams object is S4 and the species parameter data frame is S3. If we switch from S4 to S3 for everything, the syntax would change to params$species_params$w_mat. The effect on the user will be that they need to replace all @ signs in their code (that are not preceeded by a whitespace) with $ signs. Luckily we would be able to give them a regular expression with which they can make the change with one global search and replace. The advantage is that from then on the user will never again have to think about whether they need to use @ or $.

drfinlayscott commented 5 years ago

Emerges from shadows after lurking for years

Hi, First of all, I want to say how pleased I am to see all of the development going on with mizer. I haven't been involved with it, or with size-based models in general, for years now but I enjoy following what has been going on.

I know I haven't participated in mizer for a long time, but can I just say that moving the whole of mizer from S4 to S3 just to solve the issue of some users having older versions of objects might not be the best solution and looks a bit like using a sledgehammer to crack a nut. I haven't really used S3 classes and I am not very familiar with them but I think that they have some disadvantages that we should be aware of. On the whole, S3 isn't a very formal object oriented solution. For example, as far as I can remember S3 methods can only dispatch on the first argument whereas S4 methods can dispatch on multiple arguments. I know that S3 classes are a lot more relaxed about enforcing the types of objects that get assigned to slots (this is probably why they are seen as a potential solution to the version problem) but that relaxedness may come at a cost in terms of knowing exactly what is inside an object and can prevent the user from accidentally making a mess (unless you write a whole bunch of boilerplate type checking code). I'm not wedded to the idea of S4 classes at all (in many ways they are quite horrible, with a lot of memory overhead with all the copies they make when passing to and from methods) but I think you have to be quite careful that moving to S3 doesn't have unforseen side effects.

Unfortunately, I don't have a quick solution to the version problem other than encouraging users to update their saved objects by rerunning the scripts that created them using the new version package version. I don't know if it is possible to write some kind of upgrade script that allows people to convert objects from an older version to a new version.

Cheers

Finlay

Retreats back into shadows

On Tue, Apr 16, 2019 at 6:45 PM Gustav W Delius notifications@github.com wrote:

Currently mizer uses S4 classes for the MizerParams and MizerSim objects, instead of the more usual S3 methods. S4 classes are more strictly enforcing the structure of the object. This has the unfortunate consequence that if a user saves a MizerParams or MizerSim object with one version of mizer, then it can not be loaded with a later version of mizer if in that version the MizerParams class has gained extra slots, for example. We can anticipate having to add extra slots in the future for new mizer features.

Slots in S3 objects are accessed via $ whereas slots in S4 objects are accessed via @. So currently we have syntax like params@species_params $w_mat because the MizerParams object is S4 and the species parameter data frame is S3. If we switch from S4 to S3 for everything, the syntax would change to params$species_params$w_mat. The effect on the user will be that they need to replace all @ signs in their code with $ signs. Luckily we would be able to give them a regular expression with which they can make the change with one global search and replace. The advantage is that from then on the user will never again have to think about whether they need to use @ or $.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/sizespectrum/mizer/issues/56, or mute the thread https://github.com/notifications/unsubscribe-auth/ABQflsXKLDy8DvDayLTlnXvpTt9WMaJQks5vhX-kgaJpZM4cxlDQ .

gustavdelius commented 5 years ago

Hi Finlay, so nice to hear from you. And do not retreat back into the shadows! :-)

I agree with you that from the professional software engineering point of view, the more formal nature of S4 classes is an advantage, as is their more powerful dispatch mechanism. If mizer was a project with lots of software engineers working on it, I would be in favour of keeping the OO framework the way you introduced it.

However, we want mizer to be a tool for the scientists who want to easily experiment and make quick changes to the code to suit the needs of their particular research question. We want to make the hurdle as low as possible. Having to learn about S4 classes is quite a high hurdle for most people. People generally find it a lot easier to write a new function than to write a new S4 method. People certainly find it a lot easier to add another element to a list than to add another slot to an S4 class.

I know this is a trade-off, and we need to be careful to avoid anarchy while introducing freedom. But I think it is more important for the future of mizer to make it easy for newcomers to experiment with changes than it is to enforce discipline and high software-engineering standards from the outset. Yes, they will be able to make a mess of it, but that is still better than them not experimenting at all.

If we decide to merge some contributed code into core mizer that has made changes to the MizerParams or MizerSim objects, at that stage we can add code to validMizerParams() or validMizerSim() to check validity, we can add tests to the regresssion test suite to test that the constructors fill the new slots correctly and we can add code to upgradeParams() or upgradeSim() to upgrade objects from previous versions to the new version. In that way the integrity of core mizer will be maintained well even without S4 classes.

gustavdelius commented 4 years ago

This is no longer pressing. The user has now been isolated from the @ because mizer provides accessor and setter functions for all relevant slots. Extension writers will not have to create new slots, just know how to access existing slots.

juliablanchard commented 4 years ago

Sounds good!

From: Gustav W Delius notifications@github.com Sent: Thursday, April 2, 2020 9:35:59 PM To: sizespectrum/mizer mizer@noreply.github.com Cc: Subscribed subscribed@noreply.github.com Subject: Re: [sizespectrum/mizer] Stop using S4 classes (#56)

This is no longer pressing. The user has now been isolated from the @ because mizer provides accessor and setter functions for all relevant slots. Extension writers will not have to create new slots, just know how to access existing slots.

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHubhttps://github.com/sizespectrum/mizer/issues/56#issuecomment-607764257, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ABGD7OW7WYBXLLV44DCCZPDRKRTA7ANCNFSM4HGGKDIA.

University of Tasmania Electronic Communications Policy (December, 2014). This email is confidential, and is for the intended recipient only. Access, disclosure, copying, distribution, or reliance on any of it by anyone outside the intended recipient organisation is prohibited and may be a criminal offence. Please delete if obtained in error and email confirmation to the sender. The views expressed in this email are not necessarily the views of the University of Tasmania, unless clearly intended otherwise.