mcvine / acc

Accelerated mcvine engine
0 stars 1 forks source link

Refactor tests for some components to share common code #73

Closed mtbc closed 2 years ago

mtbc commented 2 years ago

Adjusts code for instruments and MCViNE comparison for,

It seemed wise to avoid touching the tapered guide while it remains actively worked on.

This work addresses but does not yet complete #69 because the common helper code will require some flexibility adding to accommodate any special needs of the tapered guide or any other current accelerated instruments. However, that improvement can be provided in subsequent PRs.

mtbc commented 2 years ago

The instrument and test for the arm component are now agreeably shorter.

ckendrick commented 2 years ago

This cleaned up the testing code a lot and makes adding test instruments much easier.

I wonder if it might be useful to move the instrument factory from tests/ to somewhere under acc/ since it could be a nice helper for constructing instruments outside of testing?

One small thing to note (which is probably another issue unrelated to this PR) is that running the test instruments with acc.run_script was giving some errors which seem to be related to adding non-acc source and monitor components to the instrument. For example, the acc compiled script for slit_instrument generates these imports:

from builtins import Sources_Source_simple as comp0
from mcvine.acc.components.optics.slit import Slit as comp1
from builtins import Monitors_Psd_monitor as comp2
from builtins import Monitors_Divpos_monitor as comp3
from builtins import Monitors_Divpos_monitor as comp4

The error comes from importing the non-acc components from builtins. If we want to test instruments with more than one acc component, then we might need to adjust the instrument factory or make one for acc components only.

mtbc commented 2 years ago

The more I look, the more it does look to be a deeper issue with my not quite yet seeing all the cogs and wheels, also maybe something to solve outwith this PR. Depends if we want to get it in now given all the files it touches and that unit tests pass, then fix after, or hold off because it breaks some stuff outwith our automated testing.

A simple script to show the issue with mixing the acc and non-acc:

from mcvine.acc import run_script
run_script.run("tests/components/optics/slit_instrument.py", "out.tmp/", 1e5, is_acc=True)

First, we have to move instrument_factory.py from tests to acc so the test instruments can see it. Could some adjustment to project setup be warranted? (I don't mind moving it, was thinking to do it by need though, to prevent providing something we may not want to commit to.) Then, we get errors like,

ImportError: cannot import name 'Sources_Source_simple' from 'builtins' (unknown location)

Thanks to @ckendrick for pointers here. Right now I don't know the path forward, any thoughts?

yxqd commented 2 years ago

Thank you @ckendrick and @mtbc about the discussion on the problem with running instrument of mixed components with mcvine.acc.run_script. Created a new issue: #76

yxqd commented 2 years ago

@mtbc nice refactor!