Skip to content

Implemented targetVolume 8#4

Closed
ryonamin wants to merge 7 commits intotkLayout:masterfrom
ryonamin:mydev2
Closed

Implemented targetVolume 8#4
ryonamin wants to merge 7 commits intotkLayout:masterfrom
ryonamin:mydev2

Conversation

@ryonamin
Copy link

@ryonamin ryonamin commented Apr 2, 2015

This includes a config file modification by Gabrielle.

@alkemyst
Copy link
Contributor

alkemyst commented Apr 2, 2015

Dear Ryo,
(sorry for the double message, I am slowly getting used to gitHub)
first of all thank you very much for this addition. I actually have a couple of comments regarding this pull request on possible improvements.

  1. the object we are talking about is called "support plate". It would be nice to change the name from bottomHybrid to something generic, like 'bottomVolume' or specific like 'supportPlate' in the code (as you like).
  2. as for the thickness, I think it would be more appropriate to define a property named supportPlateThickness in the module, and the access it, just like all of the other mechanical measurements in the module up to now (size of hybrids, size of the sensor, sensor spacing, sensor thickness, etc...).
  3. the support plate might or might not be there, depending on the module type. Would it be possible to place it or skip it, based on whether any material has a targetVolume 8 or not? Or anternatively on whether the sensorPlateThickness parameter (described in point 2) is zero or not?

Please let me know what you think of this and whether you need any help/additional info.

Thank you very much
Stefano

@ryonamin
Copy link
Author

ryonamin commented Apr 2, 2015

Dear Stefano,
First of all, you have a nice portrait :-)
Thank you for the comments.

Yes, I agree. I would take 'supportPlate', which is more informative name.

Okay, I will try to implement it, too.

Currently if you don't specify any material to targetVolume 8 in your conf file, you will not have the 'supportPlate' volume in your output xml.
Do you think this is good enough?

So I will improve the above points, please reject this PR. I will make another PR later.

Best regards,
Ryo

@alkemyst
Copy link
Contributor

alkemyst commented Apr 2, 2015

Thanks a lot. Again please let me know if you need any help with this!

Cheers
Stefano

@alkemyst alkemyst closed this Apr 2, 2015
aehart pushed a commit to OSU-CMS/tkLayout that referenced this pull request Oct 3, 2015
alkemyst pushed a commit that referenced this pull request Jan 25, 2016
Update fork from official version - dev branch
alkemyst added a commit that referenced this pull request Jul 19, 2016
alkemyst pushed a commit that referenced this pull request Dec 14, 2023
Merging overlap fix in pixel on master
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants