search

mhoffman / kmos

kMC on steroids: A vigorous attempt to make lattice kinetic Monte Carlo modelling easier
http://mhoffman.github.com/kmos/
GNU General Public License v3.0
53 stars 34 forks source link

Long process names break compile #39

Closed jmlorenzi closed 8 years ago

jmlorenzi commented 8 years ago

I have stumbled upon this while working on the otf backend but it extends to local_smart and lat_int: If you try to use very long names for processes (may also happen for other of the model components...), specifically >63 characters for local_smart, >41 for lat_int, and currently >25 characters for otf, compilation fails.

This is because process names form part of fortran function names in the source code and at some point you hit some function name length limit. Or that is what I presume (http://stackoverflow.com/questions/3967247/routine-name-length-in-fortran90)

Could you please check if you can reproduce?

This can be attenuated (I hope) by shortening a lot of names in the code, i.e.

get_rate_ -> gr_ rate_ -> r_ proclist_parameters.f90 -> proclist_pars.f90 (f2py appends the module name to the functions in an intermediate step)

Arbitrarily long names could be supported if we change paradigm and name numerate processes and go from subroutine run_CO_oxidation_bridge_top to subroutine run_p00015 but I do not think we want to do that.

I would be content to do the changes mentioned above, which would extend the maximum process name length for otf to around 38. But then I think we should include a limit on the length of the process names at xml / ini generation time. I say we go for 35 chars max.

mhoffman commented 8 years ago

I am definitely aware of that limitation. I agree that a non-descriptive short-hand like p00015 should only be used as the ultimate fall-back. When you say "35 chars max", is that the best we can do using the abbreviations you suggest (`getrate -> gr_')? I would think abbreviating frequent parts of function names are ok, as long we mention it in comments and/or reference.

When you say limit the length: what do you think of spitting out warnings when the user supplied name is too long? Another approach would be to abbreviate the reaction name in a 3-step process: first try the user-supplied as is. If it is too long try to abbreviate using from simple scheme (i.e. CO_oxidation_bridge_top could become co_b_o2e_e which could be derived entirely from the conditions/actions. And thirdly if that it is still too long or creates clashes we could go to p000015 along with appropriate warnings and comments.

Automatizing this abbreviation process will be useful if the kmos model is somehow auto-generated.

jmlorenzi commented 8 years ago

Yes, 35 chars is basically the limit with the abbreviations.

Yes I was thinking raising UserWarnings when the model is saved and/or before export.

The abbreviating idea is nice, did not think of that. Another possibility is trimming the name and adding number to avoid collision. I.e. CO_oxidation_bridge_top --> CO_oxi_00024. This way you presumaby preserve some info about what the process is.

mhoffman commented 8 years ago

Ok, I just pushed a quick hack to shorten the length of process names. It follows the scheme you suggest just above. I just set the default length to 95 in order to not break existing test. But you can change to something shorter using kmos export -l 15 ....

The rate-constant evaluation functions look .... really elegant, well done! it is both very flexible and easy to use. I don't think I would have come up with that.

Your modification of base.get_rate() however breaks the function signature in the sense that model.print_accum_rate_summation() doesn't work in the otf backend. You have a quick idea how to fix that, either by changing Model.print_accum_rate_summation or offering an alternative version of base.get_rate ?

jmlorenzi commented 8 years ago

In the commit it looks like the default is 15... or am I reading something wrong?

Thank you! I am very happy you like it.

Yes, sorry I did not fix this... My plan was to modify print_accum_rate_summation, but I never got around to it.

mhoffman commented 8 years ago

95 or 15, yes, I guess I forgot one push.

I tried to fix the base.get_rate making the site an optional argument but at least initially all rate constants are 0. Am I missing something? Is there an extra step required to initialize them? Steps to reproduce

cd [PROJECTROOT]/kmos/examples kmos export -botf -l 15 -o CO_oxidation.ini cd CO_oxidation_otf kmos shell model.rate_constants

It shows that all rate constants are zero.

I would expect that they are initialized to non-zero values.

jmlorenzi commented 8 years ago

Cannot reproduce (https://xkcd.com/583/) I have still not merged your changes (sorry! lots of work for the paper now), but while sitting in your backend-otf branch, I tried to export the .ini file and cannot even compile. I suspect that the problem is related to lines

otf_rate = None

In the .ini file. Is it possible that that None is interpreted a "None" string instead of a built-in None?

By the way, the .xml export works fine.

mhoffman commented 8 years ago

Never apologize for working on a paper!

Ok, my bad. I realized that you store the sum of available rate_constants in the volume+1 column. I have now adjusted set_rate to return that field if site_nr is not provided and touched up print_accum_rate_summation accordingly.

I also fixed the interpretation of otf_rate = None. Your hunch was absolutely right. Export and compilation should work either way now.

When you have to merge into your backend-otf branch would be great but only if you have time. I guess you can then finally close this issue.