Composite Prime Moduli Sampling and Scaling Factor Calculations (#910 phase 2) #929
Conversation
fdiasmor
changed the title
| @@ -66,6 +66,9 @@ | |||
| #include <algorithm> | |||
| #include <unordered_map> | |||
| #include <set> | |||
| #ifdef DEBUG_KEY | |||
There was a problem hiding this comment.
this change is not needed
| @@ -2339,7 +2342,10 @@ class CryptoContextImpl : public Serializable { | |||
| Ciphertext<Element> Rescale(ConstCiphertext<Element> ciphertext) const { | |||
| ValidateCiphertext(ciphertext); | |||
|
|
|||
| return GetScheme()->ModReduce(ciphertext, BASE_NUM_LEVELS_TO_DROP); | |||
| const auto cryptoParams = | |||
There was a problem hiding this comment.
This comment is for all other changes in src/pke/include/cryptocontext.h and some other places where compositeDegree is required.
I think that there could be a new function GetCompositeDegree in CiphertextImpl or, at least, a private/protected function in CryptocontextImpl GetCompositeDegreeFromCtxt to be used within CryptocontextImpl. Something like this:
uint32_t GetCompositeDegree(CALLER_INFO_ARGS_HDR) const {
const auto cryptoParams =
std::dynamic_pointer_cast<CryptoParametersRNS>(GetCryptoContext()->GetCryptoParameters());
if (!cryptoParams) {
std::string errorMsg(std::string("std::dynamic_pointer_cast<CryptoParametersRNS>() failed") + CALLER_INFO);
OPENFHE_THROW(errorMsg);
}
return cryptoParams->GetCompositeDegree();
}
so, instead of adding the same lines we could see simple changes like this:
return GetScheme()->ModReduce(ciphertext, ciphertext->GetCompositeDegree());
and in the functions with a CKKS check the change would look like:
uint32_t lvls = isCKKS(m_schemeId) ? ciphertext->GetCompositeDegree() : BASE_NUM_LEVELS_TO_DROP;
return GetScheme()->ModReduce(ciphertext, lvls);
| @@ -141,10 +166,268 @@ bool ParameterGenerationCKKSRNS::ParamsGenCKKSRNS(std::shared_ptr<CryptoParamete | |||
|
|
|||
| uint32_t dcrtBits = scalingModSize; | |||
|
|
|||
| if (scalTech == COMPOSITESCALINGAUTO || scalTech == COMPOSITESCALINGMANUAL) { | |||
| numPrimes *= compositeDegree; | |||
| std::cout << __FUNCTION__ << "::" << __LINE__ << " numPrimes: " << numPrimes << " qBound: " << qBound | |||
There was a problem hiding this comment.
This cout statement should be removed
| if (scalTech == COMPOSITESCALINGAUTO || scalTech == COMPOSITESCALINGMANUAL) { | ||
| if (compositeDegree > 2 && scalingModSize < 55) { | ||
| OPENFHE_THROW( | ||
| "COMPOSITESCALING Warning: There will probably not be enough prime moduli for composite degree > 2 and scaling factor < 55, prime moduli too small. Prime moduli size must generally be greater than 22, especially for larger multiplicative depth. Try increasing the scaling factor (scalingModSize) or feel free to try it using COMPOSITESCALINGMANUAL at your own risk."); |
There was a problem hiding this comment.
Can this string be split into lines so that each line has no more than 120 characters?
|
|
||
| if (registerWordSize < 24 && scalTech == COMPOSITESCALINGAUTO) { | ||
| OPENFHE_THROW( | ||
| "Register word size must be greater than or equal to 24 for COMPOSITESCALINGAUTO. Otherwise, try it with COMPOSITESCALINGMANUAL."); |
There was a problem hiding this comment.
Can this string be split into lines so that each line has no more than 120 characters?
| std::unordered_set<uint64_t> moduliQRecord; | ||
|
|
||
| // Sample q0, the first primes in the modulus chain | ||
| NativeInteger q; |
There was a problem hiding this comment.
NativeInteger q should be declared in the for loop below
| remBits -= std::ceil(std::log2(q.ConvertToDouble())); | ||
| } | ||
|
|
||
| std::vector<NativeInteger> qPrev(std::ceil(static_cast<double>(compositeDegree) / 2)); |
There was a problem hiding this comment.
qPrev and qNext should be inside of "if (numPrimes > 1) {"
| sf *= moduliQ[numPrimes - d].ConvertToDouble(); | ||
| } | ||
|
|
||
| uint32_t cnt = 1; |
There was a problem hiding this comment.
make the cnt variable's type boolean as it can be either 1 or 0 and, possibly, because of that change its name
| double primeProduct = 1.0; | ||
| std::unordered_set<uint64_t> qCurrentRecord; // current prime tracker | ||
|
|
||
| for (uint32_t step = 0; step < static_cast<uint32_t>(qPrev.size()); ++step) { |
There was a problem hiding this comment.
it is recommended that array indices in "for" loops should be of the size_t type. in this case "step" is an index. if step is a size_t variable, you don't need to cast qPrev.size()
| if (GetScalingTechnique() == COMPOSITESCALINGAUTO || GetScalingTechnique() == COMPOSITESCALINGMANUAL) { | ||
| usint compositeDegree = GetCompositeDegree(); | ||
| switch (compositeDegree) { | ||
| case 0: // not allowed |
There was a problem hiding this comment.
should an exception be thrown for case 0?
and the entire code block could be wrapped in to a small separate function (which could be defined as a helper function in this file only) as the code is used in 2 places
| @@ -2372,11 +2387,19 @@ class CryptoContextImpl : public Serializable { | |||
| void ModReduceInPlace(Ciphertext<Element>& ciphertext) const { | |||
| ValidateCiphertext(ciphertext); | |||
|
|
|||
| GetScheme()->ModReduceInPlace(ciphertext, BASE_NUM_LEVELS_TO_DROP); | |||
| if (isCKKS(m_schemeId)) { | |||
There was a problem hiding this comment.
Any CKKS-specific code should be implemented inside the folder for the CKKS scheme (not in global cryptocontext code)
| Ciphertext<Element> ciphertext = EvalMult(ciphertext1, ciphertext2); | ||
| algo->KeySwitchInPlace(ciphertext, evalKey); | ||
| ModReduceInPlace(ciphertext, BASE_NUM_LEVELS_TO_DROP); | ||
| uint32_t levelsToDrop = BASE_NUM_LEVELS_TO_DROP; | ||
| if (cc->getSchemeId() == CKKSRNS_SCHEME) { |
There was a problem hiding this comment.
Why is this code here rather then in CKKS files?
No description provided.