Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add CIP cartrdige infos #10

Open
wants to merge 2 commits into
base: develop
Choose a base branch
from

Conversation

retn
Copy link
Contributor

@retn retn commented May 23, 2019

Here is the cartridge data for most of the CIP cartridges.
Some data is currently missing, i will add this later on.

As the data is extracted by a program the order of the cartridges has been changed.

@erichiggins
Copy link
Contributor

@retn Thank you so much for pulling this data in -- this is a huge contribution!

It'll take me a little while to review since there is so much data. In the meantime, would you mind detailing your process for obtaining it so that I can add it to the docs in case we need to repeat the process in the future?

@retn
Copy link
Contributor Author

retn commented May 24, 2019

@erichiggins I just realised that there is a error in the created data, currently the rifle cartridges file also includes rimfire cartridges for example. I will fix this hopefully right a way and provide you the correct files

Sure i wanted to do so, i will describe it here quickly how i got it but i will also provide the source code when i cleaned it.
Currently it's a bit of a mess to be true ;)

The process:

  • Fetch the cip website for each cartridge type ( https://bobp.cip-bobp.org/en/tdcc_public?page=1&cartridge_type_id=1 )
  • Extract all Cartridges from the table with XPath and download the file
  • Extract the text of the PDF with Apache PDFBox
  • Parse the file for L6 (cartridge oal) and G1 for caliber size
  • Write into cartridges files
    That's it mostly.

Do you know if the NATO/SAAMI data is published like this? For SAAMI i currently only find the data in pictures which makes it pretty hard to extract.

@retn retn force-pushed the feature/Add_CIP_Info branch from 9d312fe to f5490f0 Compare May 24, 2019 19:20
@retn
Copy link
Contributor Author

retn commented May 24, 2019

@erichiggins Have it fixed now, though happy reviewing 😃

Copy link
Contributor

@erichiggins erichiggins left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really sorry for the delay!
Just some quick feedback before I get too deep into this.

data/pistol/cip.json Outdated Show resolved Hide resolved
"names" : [ ],
"specs" : {
"coal_mm" : 22.7,
"coal_inch" : 0.8937012699999999
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you convert the MM value to obtain the INCH value?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes exactly, had hoped that i wouldn't run here into that presentation problem with double.
Had extra checked it for multiple cartridges and there it looked okay.

Best thing would be to remove it i think, or do you have any ideas how we can work around this issue?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's fine to convert it, but there are some gotchas. Doing so directly in a programming lang can cause issues because floating point numbers don't compute like a calculator. For example, this is what a calculator will provide:

22.7 / 25.4 = 0.8937007874

Often, there will be a library available for unit conversion. (Python: pint, JS: Math.js)

I recommend using something like that, then spot-checking as many as you can where the result is predictable. I'm not exactly sure what level of precision it should be rounded too -- maybe compare with the SAAMI data?

@retn
Copy link
Contributor Author

retn commented Sep 18, 2019

Hi Erich,

sorry that it took some time to get further on this topic.
I changed to a String representation for the lengths like we already discussed.

But I also decided to remove the inch conversion for now, as I'm not getting reliable results from the conversion.
I'm always left with a conversion error, what i saw is that i would need to get up to way over 12 numbers after the dot to be able to do a correct back conversion.

We probably should be fetching this information from the SAAMI specs and use that as the value.

If you, or someone else, knows a proper Java library for that I will be more than happy to put the information back.

Best
Peter

@erichiggins
Copy link
Contributor

Hey Peter, thanks for following up!

But I also decided to remove the inch conversion for now, as I'm not getting reliable results from the conversion.

Sure, I think that's fine. The ideal approach is to have both and perhaps use some build scripts to verify and or provide what's missing. The project just isn't to that stage yet.

We probably should be fetching this information from the SAAMI specs and use that as the value.

I agree that this library should always just reflect the source material. It gets tricky when the source material has gaps.

If you, or someone else, knows a proper Java library for that I will be more than happy to put the information back.

I don't know about Java, but @GAO8A did do some work on extracting the data previously. I may try reaching out to the SAAMI folks directly to see if it'd be possible to get a plaintext version to support this library without having to jump through so many hoops.

Thank you again for your contribution! I'll try to do another review pass this week.

@erichiggins erichiggins self-assigned this Sep 23, 2019
@johanoskarsson
Copy link

I was looking for a list of cartridges in a sensible format when I ran across this project. I noticed the cartridges in the main development branch are lacking some details, so I figured I'd submit a PR. However it turns out this one already existed with most of the work already done. Any chance this one can get merged? I'd be happy to take a stab at doing something similar for SAAMI if there is appetite for more data.

@erichiggins
Copy link
Contributor

@johanoskarsson I'll have to refresh my memory on this PR and determine if any more changes are needed before it can be merged. In the meantime, you may be able to base your own branch off of this one for any changes that you'd like to make.

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