ModellingWebLab / chaste-codegen

Code generation for Chaste based on cellmlmanip
Other
1 stars 1 forks source link

Split CG: Move Printer to cellmlmanip, move weblab stuff into `cg`, keep this as a chaste code generator #54

Closed MichaelClerx closed 4 years ago

MichaelClerx commented 4 years ago

Lots of questions! Mostly for @jonc125 :smile:

MichaelClerx commented 4 years ago

The reason I think it makes sense for fc to be "smart" is that it reads fc protocol files to get information about which units it wants etc. It also reads the cellmlmanip model. It could then either do things itself, or ask cg or cellmlmanip but that means some more messy communication.

But for chaste I guess desires outputs and units etc. are all hardcoded?

MauriceHendrix commented 4 years ago

The reason I think it makes sense for fc to be "smart" is that it reads fc protocol files to get information about which units it wants etc. It also reads the cellmlmanip model. It could then either do things itself, or ask cg or cellmlmanip but that means some more messy communication.

But for chaste I guess desires outputs and units etc. are all hardcoded?

Yes at the moment in the dictionaries at the top so they're easy to find and change.

MichaelClerx commented 4 years ago

It might even make sense to decide cg should be part of cellmlmanip - but only as a lightweight templating tool, maybe including a python printer, maybe just having a printer interface that users need to supply an implementation for.

Then fc could have its own templates and printer living inside fc, and the chaste code generator could have its templates, printer, and logic living inside a chaste code generating tool?

MichaelClerx commented 4 years ago

The design of chaste_cg was always a bit up in the air :sweat_smile: Now that it's a bit more clear how we're using it's beginning to look like two projects in the same repo, with only a small bit of shared code. So seems to logical design is to merge that shared code with cellmlmanip and then split the project in two?

mirams commented 4 years ago

My gut instinct is "when it's finished"! That way you don't have to keep two things that use cellmlmanip tested and in sync with changes in cellmlmanip, and you don't have people breaking chaste_cg underneath Maurice, or vice-versa. It should be pretty similar with Chaste cg asking for certain interface units etc. as a special case of the more general weblab_cg.

jonc125 commented 4 years ago

I'd agree with @mirams not to restructure the repos yet! Although moving the common cg code into cellmlmanip probably does make sense longer term, with a basic printer to subclass, and the WebLab-specific code gen into weblab-fc. It's not clear where the Chaste-specific code gen will go - maybe into Chaste itself, for instance?

As for specific questions:

MichaelClerx commented 4 years ago

I think it'd make everyone's life a bit easier to do this soonish though. Maurice would go from working on "cg + cellmlmanip" to "chaste_cg + cellmlmanip" and not have to worry about WL stuff inside "chaste_cg". For FC it would mean that all the stuff related to FC model code (the model base class and the templates that create models that implement it) are in one place

MichaelClerx commented 4 years ago

We could even just rename this repo to chaste_cg or something similar, and slowly move other things out

jonc125 commented 4 years ago

I'd be OK with moving WebLab-specific stuff into weblab-fc as convenient. Let's avoid renaming repos and moving Maurice's stuff yet though.

MauriceHendrix commented 4 years ago

Weglab_cg feels like a decent enough fit to me since we're reusing most of the printing and using the same templates. Also if we either want others to use cellmlmanip or the other extreme replace it with libcellml it might be quite convenient for me not being the same repo

MichaelClerx commented 4 years ago

I'd be OK with moving WebLab-specific stuff into weblab-fc as convenient. Let's avoid renaming repos and moving Maurice's stuff yet though.

Half-doing stuff is the last thing I want! Desperately seeking some clarity in this project hence these proposals :-)

MichaelClerx commented 4 years ago

Also if we either want others to use cellmlmanip or the other extreme replace it with libcellml it might be quite convenient for me not being the same repo

Just to clarify my suggestion:

So you wouldn't have any chaste stuff in the cellmlmanip repo!

And there's no plan to replace cellmlmanip with libcellml, only to replace the parser class in cellmlmanip with something that uses libcellml

MauriceHendrix commented 4 years ago

Also if we either want others to use cellmlmanip or the other extreme replace it with libcellml it might be quite convenient for me not being the same repo

Just to clarify my suggestion:

  • The only bits of code the chaste and weblab code generators currently share are the method cg.load_template and the cg.Printer class. I'm suggesting these two move to cellmlmanip.
  • This means that others who want to use cellmlmanip can still do so, and can now also use load_template and the Printer for their own code generation
  • The weblab templates would move into fc
  • The chaste templates would stay here (and then this would become the chaste code generation repo) or move into some other repo (and then this repo would be deleted)

So you wouldn't have any chaste stuff in the cellmlmanip repo!

And there's no plan to replace cellmlmanip with libcellml, only to replace the parser class in cellmlmanip with something that uses libcellml

Aah I suppose that's a decent idea. I would probably prefer chaste_cg code not to be developed in the chase repositories for now as a) we'd have 2 different code generators there to confuse everybody and b) changes would trigger fairly slow C++ tests rather than out python tests which are much quicker and give me more useful errors during dev

MichaelClerx commented 4 years ago

That makes sense to me!

MichaelClerx commented 4 years ago

Another good reason for splitting now would be that Maurice gets a place to explain derived quantities etc. and other concepts that exist in chaste generation, and can do nice examples and documentation and stuff. I think this is pretty important if you want to avoid another big rewrite in a couple of years

jonc125 commented 4 years ago

I'm becoming convinced 😄 on the basis that it's a quick change to make and has several benefits. I'll try to look at it a bit more next week - I don't think we should do this before Christmas, let's get the current in-progress PRs merged, but could do in early Jan.

MauriceHendrix commented 4 years ago

Actually I'm only working 2 more days this year and I need to catch up with admin stuff & stuff for my other team so pretty much done programming for 2019 after today :)

MichaelClerx commented 4 years ago

(Worth noting that if you rename a repository, github redirects the old URLs to the new one for a very long time, so won't break any existing checkouts etc. for a few months at least)

MichaelClerx commented 4 years ago

One day of early Jan left :D

MichaelClerx commented 4 years ago

Are we still in favour of this change? @jonc125 I think I am!

MauriceHendrix commented 4 years ago

I think so and it's probably a task easier done nest week when we're all in the same room.

jonc125 commented 4 years ago

The repos are both in a fairly clear state too - no PRs open on cellmlmanip, and only 1 here.

MichaelClerx commented 4 years ago

Update:

jonc125 commented 4 years ago

I think we can close this now @MichaelClerx ?