search

TeamAtomECS / AtomECS

Cold atom simulation code
GNU General Public License v3.0
46 stars 11 forks source link

Dipole featurepack + restructure + elliptical beams #51

Closed MauriceZeuner closed 3 years ago

MauriceZeuner commented 3 years ago

Merge the other PRs before this.

This is a major feature pack that introduces a basic concept of dipole forces. Additionally, the program structure has been updated, such that we now have the modules "laser", "laser cooling" and "dipole". Also, elliptical beams were implemented, they are in this PR because they were implemented after the restructuring.

ElliotB256 commented 3 years ago

Some conflicts, possibly because of merging the other ones

MauriceZeuner commented 3 years ago

yeah, this time I can't view the conflicts in the browser, what's the procedure here? Just merging and resolving the conflicts as usual?

ElliotB256 commented 3 years ago

This is quite a large and comprehensive change, I'd like to get some numbers for a performance comparison

ElliotB256 commented 3 years ago

Now I'm back at the PC I'll add a bit more detail (writing on the phone is a pain).

Longer term, I'd like to look at writing some now detailed bench marks so we can better understand where the bottlenecks are. I'm still delaying this until after the change to whatever ECS we take (eg Legion, bevy), otherwise the rewrite will keep getting longer and longer.

In the short term, I'll just run the examples and time them. This might be a good reason to get the high-level API running and merged before this one - it means all the examples can look the same regardless of changes to the back end, hence running the same examples between different versions will be easier.

MauriceZeuner commented 3 years ago

Sounds reasonable but I think what we agreed with Tiffany was that we first bring the backend to a status where one can get some results for sequencing and (at least for me) putting the API second priority. Tiffany and Ulrich (and I, consequently) would be very happy to get some efficiencies and times and stuff like that soon.

I totally get your point but as long as the examples do the same thing intrinsically/physically, I think it shouldn't matter too much if the example file varies from version to version or the connection between API and the actual code.

Would that be okay?

ElliotB256 commented 3 years ago

I agree with BEAM_LIMIT change.

Btw, the current fixed array size approach was only marginally (e.g., 30%) faster on profiling than having a component with a variable-sized (and heap allocated) Vec<>. I'd be interested to revisit in the next round of profiling and performance tweaks.

MauriceZeuner commented 3 years ago

Isn't 30% faster a lot already? Is there a way I can start profiling as well without expensive software?

MauriceZeuner commented 3 years ago

Just tested with a separate branch: As soon as serdeoutput is merged into master, this Pull Request only has one line of conflict which is trivial to resolve (just a path that has changed).

ElliotB256 commented 3 years ago

Isn't 30% faster a lot already?

It is and I would like to keep it, but it isn't an order of magnitude different. It's marginal in the sense that it may be worth sacrificing for a large increase in flexibility.

Is there a way I can start profiling as well without expensive software?

Depends on your chipset and OS. For windows and intel I am using VTune, which is free. AMD also has a free suite. There are many free linux profiling tools.