search

APSIMInitiative / ApsimX

ApsimX is the next generation of APSIM
http://www.apsim.info
Other
134 stars 162 forks source link

File conversion error in Report? #3300

Closed sno036 closed 5 years ago

sno036 commented 5 years ago

Is there an error in file conversion when Report is reporting Script variables? A line that was

[ThisIsMyManagerScript].Script.MyVariable as MyVariable

is being changed to

[ThisIsMyManagerScript]Script.MyVariable as MyVariable

i.e. deleting the ".". Is this intended?

sno036 commented 5 years ago

Report also seems to be unconcerned about mismatching braces! New or always been like that?

hol430 commented 5 years ago

Do you have the original file handy? Failing that, the upgraded file might have some clues as well.

sno036 commented 5 years ago

Actually .... I have to take the original message back! (as unlikely as it seems). The UI is not removing the "." in "].Script" it has been like that in the release for coming up to a year! Screenshot below of C:\Work\ApsimX\Tests\Simulation\SoilNitrogenPatch\SoilNitrogenPatchValidationOfPatchiness.apsimx. The odd thing (apart from the missing "." and how they cam about to be missing is ... that the simulation runs!

image

hol430 commented 5 years ago

Ah yes, I see the problem. Do you want me to commit/make a pull request for this?

Also, I have to say that SoilNitrogenPatchValidationOfPatchiness is quite an impressive file name.

sno036 commented 5 years ago

Not sure what you mean by the Pull request - of what? To my understanding that simulation is currently contributing to the testing regime. Would you mind checking that it is?

Yes - it is a name of some magnificence - at some point I will add in PatchyMcPatchFace.apsimx to accompany it.

hol430 commented 5 years ago

I can change [Reporting]Script... to [Reporting].Script... and make a pull request to get this change merged. Currently it seems to be reporting zeros for the affected lines in the report.

hol430 commented 5 years ago

The simulation is still reporting zeroes across the board, even if I add a . between [Reporting] and Script.

I don't think Apsim throws if it's unable to resolve an expression in the report, which is why no one has picked up on this until now.

sno036 commented 5 years ago

For this particular simulation if the result of the expression is zero then the test is a pass. Essentially it is a test to make sure that just having multiple patches in SoilNitrogen (i.e. after creation they are all being treated the same) then that does not affect any results. The files contains a series of simulations to make sure that remains true. I've just tested this simulation by subtracting urea from water (nonsensical I know but the result should be non-zero) and it does give non-zero outputs. This was after I manually put the "." back into the expression. BTW - it seems completely unconcerned about the absence of the "." before Script - appears that its only function is to invoke Intellisense?

I have also tested what happens if it cannot resolve the expression. If it refers to an array element out of bounds or to a variable that does not exist then it throws an exception but not until runtime.

So good to merge it I think. Thanks, Val

hol430 commented 5 years ago

I wouldn't have thought that Apsim would be able to resolve the expression if there was a . missing...the fact that it works is probably a bug.

Is it worth writing a little manager script test to make sure that these results stay all zero?

sno036 commented 5 years ago

It is part of the testing system already – you will see a test node under the database.

From: Drew Holzworth notifications@github.com Sent: Thursday, 22 November 2018 2:38 PM To: APSIMInitiative/ApsimX ApsimX@noreply.github.com Cc: Snow, Val Val.Snow@agresearch.co.nz; Author author@noreply.github.com Subject: Re: [APSIMInitiative/ApsimX] File conversion error in Report? (#3300)

I wouldn't have thought that Apsim would be able to resolve the expression if there was a . missing...the fact that it works is probably a bug.

Is it worth writing a little manager script test to make sure that these results stay all zero?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/APSIMInitiative/ApsimX/issues/3300#issuecomment-440885548, or mute the threadhttps://github.com/notifications/unsubscribe-auth/ALnLjWmb6OYOsRnr4RCIXBPgSaKl5p4aks5uxgAVgaJpZM4YoPF2.

hol430 commented 5 years ago

I don't think the old Test nodes are run anymore; they weren't run for months (years?) because we didn't use the /RunTests command line switch. Last week, I started using the /RunTests switch again and changed it so that it only runs the new ITest nodes. Was told the old Test nodes aren't really used anymore. Several of them were failing because they hadn't been run in a long time.

hol430 commented 5 years ago

What follow-up do you want from this? Should Apsim throw if unable to resolve an expression in the report?

hol430 commented 5 years ago

In #4012 I changed report to throw an error if it can't resolve a variable name. This caused jenkins to light up like a christmas tree - almost every .apsimx file in version control is throwing errors left and right. I think this change is going to be more trouble than it's worth - not to mention it would be a breaking change for our users. So I'm going to close this now.